[PATCH] D46795: [clangd] Don't query index when completing inside classes

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/CodeComplete.cpp:810
+    return false;
+  auto Scope = CC.getCXXScopeSpecifier();
+  if (!Scope)
----------------
we could use a high level comment here explaining what the rest of the function is about. e.g.

// We also avoid ClassName::bar (but allow namespace::bar).

or just take the "we only query the index" comment, hoist it a bit, and generalize it (to not assume we're in name:: context)


================
Comment at: clangd/CodeComplete.cpp:826
+  case NestedNameSpecifier::TypeSpecWithTemplate:
+  // Note that Identifier can only appear in dependent scopes and they never
+  // refer to namespaces.
----------------
(nit: this seems like undue weight, and could just be a trailing line comment `// Unresolved inside a template`)


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:829
+TEST(CompletionTest, NoIndexCompletionsInsideClasses) {
+  // clang-format off
+  auto Completions = completions(R"cpp(
----------------
Can we avoid disabling clang-format here? I do find it useful, and it adds noise.
IIRC moving the `.items` into the `EXPECT_THAT` results in sensible formatting.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:837
+    Foo::^)cpp",
+    {func("::SomeNameInTheIndex")}).items;
+  // clang-format on
----------------
I think you should also include "Foo::SomeNameInTheIndex" in the index. At first glance this doesn't seem to actually test all the failure modes (though I think it tested the one we actually had)


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:845
+
+TEST(CompletionTest, NoIndexCompletionsInsideDependentCode) {
+  {
----------------
I think one of these would be enough, but up to you


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46795