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