[PATCH] D124176: [clangd] Add beforeExecute() callback to FeatureModules.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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