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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
