[PATCH] D52364: [clangd] Initial supoprt for cross-namespace global code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is really cool!
>From reading the code this behaves exactly how I'd expect.
Ranking will be the real test.

Main comment is I'd like to tweak the interface on FuzzyFindRequest, rest is basically nits.



================
Comment at: clangd/CodeComplete.cpp:367
     if (C.IndexResult) {
-      Completion.Origin |= C.IndexResult->Origin;
+      const auto &Sym = *C.IndexResult;
+      Completion.Origin |= Sym.Origin;
----------------
FWIW, I find "C.IndexResult" clearer than "Sym" here, the "index" part is relevant


================
Comment at: clangd/CodeComplete.cpp:375
+        Completion.Name = Sym.Name;
+      // To avoid inserting unnecessary qualifiers (e.g. no need for qualifier
+      // when symbol is accessed via using shadow like "using ns::X;"), only
----------------
Consider inverting this comment, I found it a bit hard to unpack:
```
// If the completion was visible to Sema, no qualifier is needed.
// This avoids unneeded qualifiers in cases like with `using ns::X`.
```


================
Comment at: clangd/CodeComplete.cpp:378
+      // insert qualifier if the symbol is not already available form Sema.
+      // FIXME(ioeric): find a better way to avoid inserting redundant
+      // qualifiers.
----------------
I'm not sure this FIXME is important enough to ever get fixed, consider removing it.


================
Comment at: clangd/CodeComplete.h:105
+
+  /// Whether to include index symbols that are not defined in the scopes
+  /// visible from the code completion point (e.g. enclosing namespaces). Such
----------------
This comment should also mention that this applies in contexts without explicit scope qualifiers.


================
Comment at: clangd/CodeComplete.h:106
+  /// Whether to include index symbols that are not defined in the scopes
+  /// visible from the code completion point (e.g. enclosing namespaces). Such
+  /// completions can insert scope qualifiers.
----------------
nit: I'd remove the parenthetical, it's a little ambiguous whether it applies to the clause inside the "not" or outside it.


================
Comment at: clangd/CodeComplete.h:108
+  /// completions can insert scope qualifiers.
+  bool IncludeIndexSymbolsFromAllScopes = false;
 };
----------------
what about just calling this `AllScopes`?


================
Comment at: clangd/index/Index.h:430
   ///
-  /// The global scope is "", a top level scope is "foo::", etc.
+  /// The global scope is "", a top level scope is "foo::", etc. "*" is
+  /// wildcard.
----------------
I'm not a big fan of this magic value, it seems too easy to forget to handle.
Especially since we have a lot of existing code that touches this interface, and we may forget to check some of it.

I suggest making this a separate boolean field `AnyScope`, with a comment that scopes explicitly listed will be ranked higher.
We can probably also remove the "If this is empty" special case from `Scopes` now too, as the old behavior can be achieved by setting `Scopes = {}; AnyScope = true;`


================
Comment at: clangd/index/Index.h:432
+  /// wildcard.
+  /// FIXME: support assigning different weight to each scope.
   std::vector<std::string> Scopes;
----------------
May not want a heavyweight API with explicit weights...

I think what we really **need** here is the ability to weight `clang::clangd::` > `clang::` > `` when you're in the scope of namespace clangd.

We could just say something like "the primary scope should come first" and do some FileDistance-like penalizing of the others...


================
Comment at: clangd/index/dex/Dex.cpp:161
+    if (Scope == "*") {
+      ScopeIterators.push_back(createTrue(Symbols.size()));
+      continue;
----------------
wrap in a `createBoost(..., 0.1)` or so?


================
Comment at: clangd/tool/ClangdMain.cpp:139
 
+static llvm::cl::opt<bool> CrossNamespaceCompletion(
+    "cross-namespace-completion",
----------------
It's a little confusing to give these different internal vs external names - do we have a strong reason not to call this "all-scopes-completion" or so?


================
Comment at: clangd/tool/ClangdMain.cpp:142
+    llvm::cl::desc(
+        "This is an experimental feature. If set to true, code completion will "
+        "include index symbols that are not defined in the scopes (e.g. "
----------------
I'm not sure whether putting "experimental" in every flag description is worthwhile - maybe hidden is enough?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:2043
 
+TEST(CompletionTest, CrossNamespaceCompletion) {
+  clangd::CodeCompleteOptions Opts = {};
----------------
also verify that `na::Clangd^` only gets one result?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:2059
+                                   AllOf(Qualifier("ny::"), Named("Clangd2")),
+                                   AllOf(Qualifier(""), Named("Clangd3")),
+                                   AllOf(Qualifier("nb::"), Named("Clangd4"))));
----------------
can you add the scope matcher on this line for clarity?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:2073
+                             {cls("nx::Clangd1"), cls("nx::Clangd2")}, Opts);
+  EXPECT_THAT(Results.Completions,
+              UnorderedElementsAre(AllOf(Qualifier(""), Named("Clangd1")),
----------------
maybe add a comment reiterating what this is testing: "Although Clangd1 is from another namespace, Sema tells us it's in-scope and needs no qualifier."


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52364