[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.

[Sam McCall via Phabricator via cfe-commits]

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