[PATCH] D97555: [clangd] Add diagnostic augmentation hook to Modules

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 4 01:59:31 PST 2021


kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Module.h:97
 
+  /// Called by auxilary threads as diagnostics encountered to generate fixes.
+  /// So this can be called concurrently from multiple threads. If a module
----------------
sammccall wrote:
> This seems too specialized to me - maybe we want to observe diagnostics only, or mutate some other property of them.
> 
> Consider just passing in the (mutable) clangd::Diag, and returning void? Maybe call it `handleDiagnostic` or so
> 
> Maybe also the clang::Diagnostic if you really need it.
passing in `clangd::Diag` and mutating it sounds pretty neat, thanks!

I do think it is also useful to pass `clang::Diagnostic` though, as it usually contains extra semantic information that might be used to generate a fix.
For example in case of missing module deps:
- it knows about the particular include/import that triggered the diagnostic
- name of the current module.

The former can be inferred from diagnostic range and reading the source code (we'll need to pass in sourcemanager somehow though, to ensure we read the same version of the file), and the latter can be inferred by looking at the compile commands (or implementations can query the build system with TUs name etc.). But this is one particular case, it might tie our hands when we want to generate fixes that require deeper semantic analysis (like type info) in the future. So i think we should really be passing `clang::Diagnostic` too.

So I'll update this to take in a read-only `clang::Diagnostic`, `Level` and a mutable `clangd::Diag`, does that SG?


================
Comment at: clang-tools-extra/clangd/Module.h:98
+  /// Called by auxilary threads as diagnostics encountered to generate fixes.
+  /// So this can be called concurrently from multiple threads. If a module
+  /// provides a code action with a custom, it should also register itself as
----------------
sammccall wrote:
> random thoughts about how this gets called... (definitely not for now though!)
> 
> There are other things we might want to do while building ASTs.
> e.g. *generate* diagnostics (maybe IWYU or clang-tidy can be modules)
> At that point maybe we want something like
> ```
> virtual unique_ptr<ParseASTHooks> Module::parseAST() { return nullptr; } // threadsafe
> class ParseASTHooks { // implementations need not be threadsafe
>   virtual void sawDiagnostic(clangd::Diag &D);
>   virtual void finish(ParsedAST&);
> };
> ```
> That way it's easier to reason about call sequences. (and it eliminates the problem of calling an empty virtual method on each module for each diagnostic)
SG indeed, I'd be happy to do that in a follow-up change.

Though this might still have the problem of calling an empty virtual method with each diag, for example if a module doesn't do anything per-diagnostic but only does something on `finish`, but it definitely limits the scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97555



More information about the cfe-commits mailing list