kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
Comment at: clang-tools-extra/clangd/FeatureModule.h:109
+ // Called before the preamble build. Allows modules to modify the
+ // CompilerInvocation, prefetch required files, etc.
----------------
nit: triple slashes
================
Comment at: clang-tools-extra/clangd/FeatureModule.h:109
+ // Called before the preamble build. Allows modules to modify the
+ // CompilerInvocation, prefetch required files, etc.
----------------
kadircet wrote:
> nit: triple slashes
comment still says `preamble build`. I'd either talk more about `Allows
customizing the compiler instance before starting the execution on input` or
get rid of `CompilerInvocation` in that list. as in theory modifications to
only parts of it will be respected (since we've already issued beginsourcefile
on compiler instance and created some structs with whatever we had in compiler
invocation prior to this call).
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:553
+ for (const auto &L : ASTListeners)
+ L->beforeExecute(*Clang);
----------------
could you have a comment that says we must perform this after
`Action::BeginSourceFile` to ensure PP and rest of the helper structs are
initialized and closer to the end so that other modifications we do in clangd
(like adjusting diag severities/marking main file as include-guarded) is
visible to modules?
to make sure it doesn't get moved around without these in mind.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:72
+ : File(File), ParsedCallback(ParsedCallback), Stats(Stats),
+ BeforeExecuteCallback(BeforeExecuteCallback) {}
----------------
nit: std::move
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:117
void BeforeExecute(CompilerInstance &CI) override {
+ if (BeforeExecuteCallback)
+ BeforeExecuteCallback(CI);
----------------
could we move this to the bottom, to make it closer to the way parsedast calls
it?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:488
+ [&ASTListeners](CompilerInstance &CI) {
+ for (const auto &L : ASTListeners) {
+ L->beforeExecute(CI);
----------------
nit: drop braces
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124176/new/
https://reviews.llvm.org/D124176
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
