sammccall added inline comments.
================
Comment at: clangd/IncludeFixer.cpp:208
+ SM.getBufferData(SM.getDecomposedLoc(Typo.getLoc()).first, &Invalid);
+ assert(!Invalid);
// Extract the typed scope. This is not done lazily because `SS` can get
----------------
No idea when this can be invalid, but we could just return here?
================
Comment at: clangd/IncludeFixer.cpp:211
// out of scope and it's relatively cheap.
llvm::Optional<std::string> SpecifiedScope;
+ // Extra scope to append to the query scopes. This is useful, for example,
----------------
We can be more precise about this now I think: IIUC this is the **resolved**
specified scope.
in two senses: it's the part of what was typed that was resolved, and it's in
its resolved form not its typed form (think `namespace clang { clangd::x }` -->
`clang::clangd::`)
================
Comment at: clangd/IncludeFixer.cpp:212
llvm::Optional<std::string> SpecifiedScope;
+ // Extra scope to append to the query scopes. This is useful, for example,
+ // when the unresolved name is a qualier, in which case we use the name
that
----------------
nit: I don't think this is an example, it's the only case right?
Consider calling this UnresolvedSpecifiedScope
================
Comment at: clangd/IncludeFixer.cpp:213
+ // Extra scope to append to the query scopes. This is useful, for example,
+ // when the unresolved name is a qualier, in which case we use the name
that
+ // comes after it as the name to resolve and use the qualifier as the extra
----------------
nit: qualier -> qualifier
accissible -> accessible
================
Comment at: clangd/IncludeFixer.cpp:216
+ // scope in the accissible scopes.
+ llvm::Optional<std::string> ExtraScope;
if (SS && SS->isNotEmpty()) { // "::" or "ns::"
----------------
I'm a bit concerned about how this mixes extraction of scopes from the source
code/AST (which is now *really* complicated) with building the query and
assembling the correction.
Can we reasonably extract the former to a helper function? Not sure exactly
what the signature would be, but it would be helpful to find out.
The other thing is I think this has a lot in common with the scope-wrangling
code in CodeComplete, and could *maybe* be unified a bit once isolated.
================
Comment at: clangd/IncludeFixer.cpp:235
+ std::string Spelling = (Code.substr(B, E - B) + "::").str();
+ if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
+ SpecifiedScope = SpecifiedNS;
----------------
hmm, won't this heuristic have false positives?
```
// indexed-header.h
namespace a { int X; }
// main-file.cc
namespace b = a;
namespace c { int Y = b::x; }
```
I worry spelling is going to be "b::" here, while SpecifiedNS is going to be
"a::".
================
Comment at: clangd/IncludeFixer.cpp:315
private:
+ // Returns the idenfiers qualified by an unresolved name. \p Offset is the
+ // first character after the unresolved name in \p Code. For the example
----------------
identifiers
================
Comment at: clangd/IncludeFixer.cpp:320
+ // ~~~~~~
+ llvm::Optional<std::string> qualifiedByUnresolved(llvm::StringRef Code,
+ size_t Offset) const {
----------------
this only depends on the params right?
This could be static or even moved out of the class.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
