[PATCH] D57021: [clangd] Suggest adding missing includes for typos (like include-fixer).

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/IncludeFixer.cpp:209
+  std::vector<std::string> Scopes;
+  if (Typo.SS) {
+    Scopes.push_back(*Typo.SS);
----------------
sammccall wrote:
> why do we do this here rather than compute the scopes/name already in CorrectTypo()?
> 
> Passing around more of these AST objects and doing the computations later seems a bit more fragile.
Because not all typos will result in diagnostics. Computing scopes could be expensive, so we would only want to do that when necessary. For example, sema can perform typo corrections for both `x` and `y` in `x::y`, while only one diagnostic (for either `x` or `y`) would be generated.


================
Comment at: clangd/IncludeFixer.cpp:233
+
+  SymbolSlab::Builder Matches;
+  Index.fuzzyFind(Req, [&](const Symbol &Sym) {
----------------
sammccall wrote:
> why put these into a slab and then build fixes later, rather than build the fixes immediately?
good point.


================
Comment at: clangd/IncludeFixer.h:35
 public:
-  IncludeFixer(llvm::StringRef File, std::shared_ptr<IncludeInserter> Inserter,
+  IncludeFixer(CompilerInstance &Compiler, llvm::StringRef File,
+               std::shared_ptr<IncludeInserter> Inserter,
----------------
sammccall wrote:
> Having to inject the whole compiler into the include fixer doesn't seem right. Apart from the huge scope, we're also going to register parts of the include fixer with the compiler.
> 
> A few observations:
>  - all we actually need here is Sema and the SM (available through Sema)
>  - it's only the typo recorder that needs access to Sema
>  - the typo recorder gets fed the Sema instance (`ExternalSemaSource::InitializeSema`)
>  - the fact that `fixTypo()` needs to access the typo recorder doesn't seem necessary
> 
> So I'd suggest changing this function to return a *new* ExternalSemaSource that stashes the last typo in this IncludeFixer object directly.
> 
> e.g.
> 
> ```
> llvm::IntrusiveRefCntPtr<ExternalSemaSource> typoRecorder() {
>   return new TypoRecorder(LastTypo);
> }
> private:
>   Optional<TypoRecord> LastTypo;
>   class TypoRecorder : public ExternalSemaSource {
>     Sema *S;
>     Optional<TypoRecord> &Out;
>     InitializeSema(Sema& S) { this->S = &S; }
>     CorrectTypo(...) {
>       assert(S);
>       ...
>       Out = ...;
>     }
>   }
>   ```
Great idea. Thanks!

I've done most of the suggested refactoring.

> the fact that fixTypo() needs to access the typo recorder doesn't seem necessary.
Do you mean access to *Sema*? The `IncludeFixer` still needs sema because we don't want to run Lookup for each typo sema encounters. We only want to do that for typos that make it to diagnostics. See my response to the other comment about this for details.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57021