[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:237
+      if (auto Hook = Mod.astHooks()) {
+        Inputs.PreambleHooks.emplace_back(std::move(Hook));
+        Inputs.MainFileHooks.emplace_back(Mod.astHooks());
----------------
Now we're creating two ASTListeners for each version of the file seen, but most won't be parsed twice (and some won't even be parsed once).

This seems like it's going to be a weird wart for e.g. hooks that expect to do something interesting when a file finishes, or those that allocate something heavyweight.

I'd suggest sinking the ModuleSet into ParsedAST::build etc, or at least moving the astHooks() calls into TUScheduler. (And documenting that astHooks() should be threadsafe)

Otherwise, need to document that sometimes hooks are created and then never used.


================
Comment at: clang-tools-extra/clangd/Compiler.h:60
   TidyProviderRef ClangTidyProvider = {};
+  // Using shared_ptr since ParseInputs are copied between threads. But none of
+  // these hooks are accessed simultaneously.
----------------
I think having these in ParseInputs is conceptually confusing and shared_ptr is risky.
For example we copy inputs into ASTWorker::FileInputs, which extends the lifetime of the listeners in a way that seems undesirable.

I think these listeners should be clearly either:
 - bound to the lifetime of the ParsedAST (and therefore owned by it)
 - bound to the *creation* of the ParsedAST (and therefore strictly scoped within ParsedAST::build)

You could consider the **ModuleSet** to be part of ParseInputs, though...


================
Comment at: clang-tools-extra/clangd/Diagnostics.h:147
   void setLevelAdjuster(LevelAdjuster Adjuster) { this->Adjuster = Adjuster; }
+  void setASTHooks(
+      llvm::ArrayRef<std::shared_ptr<FeatureModule::ASTListener>> Hooks) {
----------------
ArrayRef<shared_ptr<ASTListener>> seems like we're leaking a lot. And even mentioning AST hooks seems dubious layering-wise.
Can we use a std::function like LevelAdjuster?


================
Comment at: clang-tools-extra/clangd/FeatureModule.h:101
 
+  /// Various hooks that can be invoked by ASTWorker thread while building an
+  /// AST. These hooks are always called from a single thread, so
----------------
nit: this describes the interaction more but not what it's for.
Maybe 
```
/// Extension point that allows modules to observe and modify an AST build.
/// One instance is created each time clangd produces a ParsedAST or PrecompiledPreamble.
/// For a given instance, lifecycle methods are always called on a single thread.
```


================
Comment at: clang-tools-extra/clangd/FeatureModule.h:113
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr<ASTListener> astHooks() { return nullptr; }
+
----------------
the "hooks" name is still used here and throughout


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499