-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[clangd] Enable parsing of forwarding functions in the preamble by default #127359
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
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Nathan Ridge (HighCommander4) ChangesFixes clangd/clangd#2324 Full diff: https://github.com/llvm/llvm-project/pull/127359.diff 1 Files Affected:
diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h
index 4e68da7610ca2..e513e4c40794a 100644
--- a/clang-tools-extra/clangd/Compiler.h
+++ b/clang-tools-extra/clangd/Compiler.h
@@ -40,7 +40,7 @@ class IgnoreDiagnostics : public DiagnosticConsumer {
// Options to run clang e.g. when parsing AST.
struct ParseOptions {
- bool PreambleParseForwardingFunctions = false;
+ bool PreambleParseForwardingFunctions = true;
bool ImportInsertions = false;
};
|
upsj
left a comment
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.
Maybe for consistency we should also change the other defaults that IIRC are used to configure tests:
| bool PreambleParseForwardingFunctions = false; |
| bool PreambleParseForwardingFunctions = false; |
0ff0680 to
fd189ad
Compare
Done |
upsj
left a comment
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.
LGTM!
hokein
left a comment
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.
The change looks good. We should be mindful of the potential performance regression due to this (1% increase in preamble size, as noted https://reviews.llvm.org/D124688#3483895).
I’ll leave the final approval to @kadircet.
kadircet
left a comment
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.
thanks, as long as we're keeping the underlying option I think this makes sense! we can flip it in production if we see big regressions
kadircet
left a comment
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.
thanks, as long as we're keeping the underlying option I think this makes sense! we can flip it in production if we see big regressions
Fixes clangd/clangd#2324