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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! The layering is *much* clearer now.
I can still suggest a couple of tweaks, but they're pretty much cosmetic.



================
Comment at: clangd/IncludeFixer.cpp:190
+//     clang::clangd::X::Y
+//            ~~~~~~
+llvm::Optional<std::string> qualifiedByUnresolved(llvm::StringRef Code,
----------------
you're showing a range here, it should be a point though?


================
Comment at: clangd/IncludeFixer.cpp:191
+//            ~~~~~~
+llvm::Optional<std::string> qualifiedByUnresolved(llvm::StringRef Code,
+                                                  size_t Offset) {
----------------
this isn't wrong per se (conservative, bails out e.g. on unicode)
But is it much harder to call Lexer::findNextToken twice and expect a raw identifier and ::?


================
Comment at: clangd/IncludeFixer.cpp:231
+    const SourceManager &SM, CXXScopeSpec *SS, llvm::StringRef UnresolvedName,
+    SourceLocation UnresolvedLoc, bool IsUnrsolvedSpecifier) {
+  bool Invalid = false;
----------------
Unrsolved -> Unresolved

emphasis might be clearer as `UnresolvedIsSpecifier` - there's always an unresolved entity, the boolean is indicating that it's a specifier


================
Comment at: clangd/IncludeFixer.cpp:256
+        std::string Spelling = (Code.substr(B, E - B) + "::").str();
+        vlog("Spelling scope: {0}, SpecifiedNS: {1}", Spelling, SpecifiedNS);
+        if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
----------------
I don't think this vlog is useful as-is (quite far down the stack with no context)
Did you intend to remove it?


================
Comment at: clangd/IncludeFixer.cpp:322
     UnresolvedName Unresolved;
     Unresolved.Name = Typo.getAsString();
     Unresolved.Loc = Typo.getBeginLoc();
----------------
Following up on our offline discussion :-)
I think that since `extractSpecifiedScopes` can want to modify the name, we should just expand that function's signature/responsibility to always determine the name.

So we'd pass the `const DeclarationNameInfo&` to `extractSpecifiedScopes`, and it would return a struct `{optional<string> ResolvedScope; optional<string> UnresolvedScope; string Name}`. 
Maybe need to call them `CheapUnresolvedName`/`extractUnresolvedNameCheaply` or  similar.
But I think the code change is small.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58185