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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
