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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/IncludeFixer.cpp:191
+//            ~~~~~~
+llvm::Optional<std::string> qualifiedByUnresolved(llvm::StringRef Code,
+                                                  size_t Offset) {
----------------
sammccall wrote:
> 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 ::?
Good idea. Lexer is a better option.


================
Comment at: clangd/IncludeFixer.cpp:251
+        //     namespace clang { clangd::X; }
+        // In this case, we use the "typo" qualifier as extra scope instead
+        // of using the scope assumed by sema.
----------------
jkorous wrote:
> Does this work with anonymous namespaces?
I'm not sure... how do you use an anonymous namespace as a scope 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))
----------------
sammccall wrote:
> I don't think this vlog is useful as-is (quite far down the stack with no context)
> Did you intend to remove it?
Indeed. Thanks for the catch!


================
Comment at: clangd/IncludeFixer.cpp:271
+  if (IsUnrsolvedSpecifier) {
+    // If the unresolved name is a qualifier e.g.
+    //      clang::clangd::X
----------------
jkorous wrote:
> Is the term `qualifier` applicable here? (Honest question.)
> 
> It seems like C++ grammar uses `specifier` (same as you in `IsUnrsolvedSpecifier `)
> http://www.nongnu.org/hcb/#nested-name-specifier
You've raised a good point. We've been using `specifier` and `qualifier` interchangeably in the project. But specifier does seem to be a more appropriate name to use.  I've fixed uses in this file. Uses in other files probably need a separate cleanup.


================
Comment at: clangd/IncludeFixer.cpp:322
     UnresolvedName Unresolved;
     Unresolved.Name = Typo.getAsString();
     Unresolved.Loc = Typo.getBeginLoc();
----------------
sammccall wrote:
> 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.
Sounds good. Thanks!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58185