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