-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang] Introduce copy-on-write CompilerInvocation
#65412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I haven't dug into this in depth yet, but did you consider keeping the normal function names for the |
I did, but decided against it. There are places in Clang where we store the result of the const function in some kind of long-lived data structure, and someplace else modify the result of the non-const function, expecting the change to be reflected. I wanted to make the semantic difference clear on both ends in case someone was inclined to swap Note that you can use |
I would be comfortable with this, because you would still have to call a distinctly named function (getMut...) in order to do the mutation itself IIUC. If they are const-casting of course that wouldn't be enough.
I think |
Ok, if you're fine with that, I'll use the original names for the const getters on
No, |
7c0af25
to
16be7a0
Compare
CompilerInvocation
I evicted a couple of commits from this PR in order to work better with the new GitHub PR "squash and merge" workflow. I'll create new PRs for the remaining commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
GenerateFileSystemArgs(getFileSystemOpts(), Consumer); | ||
GenerateMigratorArgs(getMigratorOpts(), Consumer); | ||
GenerateAnalyzerArgs(getAnalyzerOpts(), Consumer); | ||
GenerateDiagnosticArgs(getDiagnosticOpts(), Consumer, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GenerateDiagnosticArgs(getDiagnosticOpts(), Consumer, false); | |
GenerateDiagnosticArgs(getDiagnosticOpts(), Consumer, /*DefaultDiagColor=*/false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #65647.
Made the entirety of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically LGTM, but could we add some unit tests for the new type?
When adopted in the dependency scanner, this will speed up execution and reduce memory usage.
This PR introduces new copy-on-write `CompilerInvocation` class (`CowCompilerInvocation`), which will be used by the dependency scanner to reduce the number of copies performed when generating command lines for discovered modules.
This PR introduces new copy-on-write `CompilerInvocation` class (`CowCompilerInvocation`), which will be used by the dependency scanner to reduce the number of copies performed when generating command lines for discovered modules.
This PR introduces new copy-on-write
CompilerInvocation
class (CowCompilerInvocation
), which will be used by the dependency scanner to reduce the number of copies performed when generating command lines for discovered modules.