[PATCH] D52364: [clangd] Initial supoprt for cross-namespace global code completion.
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 26 07:48:33 PDT 2018
ioeric added inline comments.
================
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.
----------------
sammccall wrote:
> 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;`
sounds good.
> 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;
I think this is a good idea. Unfortunately, there seem to be many tests that rely on the old behavior. I'll add a FIXME and do it in followup patch.
================
Comment at: clangd/index/Index.h:432
+ /// wildcard.
+ /// FIXME: support assigning different weight to each scope.
std::vector<std::string> Scopes;
----------------
sammccall wrote:
> 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...
Changed the wording of `FIXME`.
> I think what we really need here is the ability to weight clang::clangd:: > clang:: > `` when you're in the scope of namespace clangd.
It's unclear what this would mean for scopes from `using-namespace` directives. But we can revisit when we get there.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52364
More information about the cfe-commits
mailing list