sammccall added inline comments.
================
Comment at: clangd/IncludeFixer.cpp:74
+ break;
+ default:
+ assert(RecordTypo);
----------------
"default" looks a bit scary - could still end up with false positives.
Can we cover a few of the most common cases (even if we know they're not
exhaustive) to start with?
================
Comment at: clangd/IncludeFixer.cpp:203
+ LastTypo = std::move(Record);
+
+ return TypoCorrection();
----------------
a comment that we never return a valid correction to try to recover, our
suggested fixes always require a rebuild.
================
Comment at: clangd/IncludeFixer.cpp:209
+ std::vector<std::string> Scopes;
+ if (Typo.SS) {
+ Scopes.push_back(*Typo.SS);
----------------
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.
================
Comment at: clangd/IncludeFixer.cpp:227
+
+ FuzzyFindRequest Req;
+ Req.AnyScope = false;
----------------
limit?
================
Comment at: clangd/IncludeFixer.cpp:233
+
+ SymbolSlab::Builder Matches;
+ Index.fuzzyFind(Req, [&](const Symbol &Sym) {
----------------
why put these into a slab and then build fixes later, rather than build the
fixes immediately?
================
Comment at: clangd/IncludeFixer.cpp:241
+ auto Syms = std::move(Matches).build();
+ if (Syms.empty())
+ return {};
----------------
Doesn't seem worth special casing this, below code will work fine.
================
Comment at: clangd/IncludeFixer.cpp:246
+ auto Fixes = fixesForSymbol(Sym);
+ Results.insert(Results.end(), Fixes.begin(), Fixes.end());
+ }
----------------
the fixes can overlap: distinct symbols may be defined in the same header.
Common case: function overloads.
At some level we "just want to deduplicate" these.
It's slightly complicated because of the tricky way we calculate the edits.
Maybe the best thing is to pass in all the symbols to fixesForSymbol() instead
of just one.
One thing to keep in mind is a future where we want to pull in results across
namespaces: the fixes for "include 'a.h' for x::Foo" and "include 'a.h' for
y::Foo" need to be distinct because of the different added qualifiers.
So it'd be good to end up with an explicit e.g. map<header_to_include, Fix> to
deduplicate, then we can just change the key to pair<header_to_include,
qualifier_to_insert>
================
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,
----------------
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 = ...;
}
}
```
================
Comment at: clangd/IncludeFixer.h:46
+ /// Returns an ExternalSemaSource that records typos seen in Sema. It must be
+ /// used in the same Sema run as the IncludeFixer.
----------------
This would be a reasonable place to document what the feature is and how it
works :-)
e.g.
```
/// Returns an ExternalSemaSource that records failed name lookups in Sema.
/// This allows IncludeFixer to suggest inserting headers that define those
names.
```
================
Comment at: clangd/IncludeFixer.h:59
+ struct TypoRecord {
+ std::string Typo; // The typo identifier e.g. "X" in ns::X.
----------------
So an annoying naming thing: I find the use of "typo" to describe this feature
really confusing. There's no actual typo, and conflating Sema's typo-correction
fixes (which allow the parse to recover) and our generated fixes (which
requires changing the source and rebuilding) breaks my brain a bit.
I'd suggest calling this "unresolved name" throughout, except when specifically
referring to the mechanism by which we observe unresolved names.
e.g. `TypoRecorder` -> `UnresolvedNameRecorder` etc.
================
Comment at: clangd/IncludeFixer.h:68
+ /// Records the last typo seen by Sema.
+ class TypoRecorder : public ExternalSemaSource {
+ public:
----------------
I think TypoRecorder can be merely declared here, and defined in the CC file?
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
