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