jkorous added a comment.
Hi Eric, I have just couple details.
================
Comment at: clangd/IncludeFixer.cpp:188
+// first character after the unresolved name in \p Code. For the example below,
+// this returns "::X::Y" that is qualfied by unresolved name "clangd":
+// clang::clangd::X::Y
----------------
qualfied -> qualified
================
Comment at: clangd/IncludeFixer.cpp:193
+ size_t Offset) {
+ auto IsValidIdentifierChar = [](char C) {
+ return ((C >= 'a' && C <= 'z') || (C >= 'A' && C <= 'Z') ||
----------------
It seems to me that calling Lexer would be better indeed.
================
Comment at: clangd/IncludeFixer.cpp:216
+ // resolved form not its typed form (think `namespace clang { clangd::x }`
-->
+ // `clang::clangd::`). This is not done lazily because `SS` can get out of
+ // scope and it's relatively cheap.
----------------
Maybe move the comment about work done eagerly to the function?
================
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.
----------------
Does this work with anonymous namespaces?
================
Comment at: clangd/IncludeFixer.cpp:263
+ Result.Resolved = printNamespaceScope(*ANS->getNamespace());
+ else
+ // We don't fix symbols in scopes that are not top-level e.g. class
----------------
I'd personally prefer to add parentheses here to have the if/else if/else
consistent. Up to you though.
================
Comment at: clangd/IncludeFixer.cpp:271
+ if (IsUnrsolvedSpecifier) {
+ // If the unresolved name is a qualifier e.g.
+ // clang::clangd::X
----------------
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
================
Comment at: unittests/clangd/DiagnosticsTests.cpp:452
+TEST(IncludeFixerTest, UnresolvedNameAsQualifier) {
+ Annotations Test(R"cpp(
----------------
If (see above) we decide `qualifier` should be replaced by `specifier` or smth
else please replace here as well, otherwise ignore this.
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
