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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a project: clang-tools-extra.

I'm getting a little nervous about the amount of stuff we're packing into modules without in-tree examples.
I should split out some of the "standard" features into modules as that's possible already.

My model for modules using this diagnostic stuff (apart from the build-system stuff which sadly can't be meaningfully upstreamed) are IncludeFixer, ClangTidy, and IWYU - worth thinking about how we'd build those on top of this model. (Doesn't mean we need to add a hook to emit diagnostics, but it probably means we should know where it would go)



================
Comment at: clang-tools-extra/clangd/FeatureModule.h:104
+  /// implementations don't need to be thread-safe.
+  struct ParseASTHooks {
+    virtual ~ParseASTHooks() = default;
----------------
naming: giving this a plural name and having a collection of them is a little confusing (is `Hooks` the hooks from one module, or from all of them?). What about ASTListener?
(Listener suggests passive but listeners that "participate" is a common enough idiom I think)


================
Comment at: clang-tools-extra/clangd/FeatureModule.h:112
+  };
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr<ParseASTHooks> astHooks() { return nullptr; }
----------------
This comment hints at what I *thought* was the idea: astHooks() is called each time we parse a file, the returned object has methods called on it while the file is being parsed, and is then destroyed.

But the code suggests we call once globally and it has effectively the same lifetime as the module.
This seems much less useful, e.g. if we want to observe several diagnostics, examine the preamble, and emit new diagnostics, then we have to plumb around some notion of "AST identity" rather than just tying it to the identity of the ParseASTHooks object itself.

(Lots of natural extensions here like providing ParseInputs to astHooks(), but YAGNI for now)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98499