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

[Jan Korous via Phabricator via cfe-commits]

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