ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
lgtm
================
Comment at: clangd/index/Index.h:158
unsigned References = 0;
-
+ /// Whether or not this is symbol is meant to be used for the global
+ /// completion.
----------------
malaperle wrote:
> sammccall wrote:
> > ioeric wrote:
> > > s/this is symbol/this symbol/?
> > Sorry, I hadn't seen this patch until now.
> > When it was part of the `workspace/symbol` patch, I was the other person
> > concerned this was too coupled to existing use cases and preferred
> > something more descriptive.
> >
> > I dug up an analysis @hokein and I worked on. One concluseion we came to
> > was that we thought results needed by `completion` were a subset of what
> > `workspace/symbol` needs, so a boolean would work. It seemed a bit ad-hoc
> > and fragile though.
> >
> > The cases we looked at were:
> >
> > ||private|member|local|primary template|template specialization|nested in
> > template|
> > | code complete |N|N|N|Y|N|N|
> > | workspace/symbol |Y|Y|N|Y|Y|Y|
> > | go to defn |Y|Y|?|?|?|?|
> > (Y = index should return it, N = index should not return it, ? = don't care)
> >
> > So the most relevant information seems to be:
> > - is this private (private member, internal linkage, no decl outside cpp
> > file, etc)
> > - is this nested in a class type (or template)
> > - is this a template specialization
> >
> > I could live with bundling these into a single property (though they seem
> > like good ranking signals, and we'd lose them for that purpose), it will
> > certainly make a posting-list based index more efficient.
> >
> > In that case I think we should have canonical documentation *somewhere*
> > about exactly what this subset is, and this field should reference that.
> > e.g. `isIndexedForCodeCompletion()` in `CodeComplete.h` with docs and
> > `IsIndexedForCodeCompletion` here. (Please avoid "global" as it has too
> > many different meanings - here we mean "index-based").
> OK, I added documentation on the SymbolCollector (which was outdated) about
> what kinds of symbols are collected, with a reference to shouldFilterDecl.
> The subset is documented on isIndexedForCodeCompletion(), also referenced
> from the Symbol field. Was that more or less what you meant?
> I could live with bundling these into a single property (though they seem
> like good ranking signals, and we'd lose them for that purpose), it will
> certainly make a posting-list based index more efficient.
FWIW, I think we could still add those signals when we need them in the future.
It seems reasonable to me for the code completion flag to co-exist with ranking
signals that could potentially overlap.
================
Comment at: unittests/clangd/CodeCompleteTests.cpp:741
+TEST(CompletionTest, Enums) {
+ EXPECT_THAT(completions(R"cpp(
----------------
malaperle wrote:
> ioeric wrote:
> > It's not clear to me what the following tests (`Enums`,
> > `AnonymousNamespace`, `InMainFile`) are testing. Do they test code
> > completion or symbol collector? If these test symbol collection, they
> > should be moved int SymbolCollectorTest.cpp
> They are testing that code completion works as intended regardless of how
> symbol collector is implemented. It's similar to our previous discussion in
> D44882 about "black box testing". I can remove them but it seems odd to me to
> not have code completion level tests for all cases because we assume that it
> will behave a certain way at the symbol collection and querying levels.
FWIW, I am not against those tests at all; increasing coverage is always a nice
thing to do IMO. I just thought it would make more sense to add them in a
separate patch if they are not related to changes in this patch; I found
unrelated tests a bit confusing otherwise.
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:141
Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+ Args.push_back(TestFileName);
+
----------------
malaperle wrote:
> This allows to override the "-xc++" with something else, i.e. -xobjective-c++
I think this could also be a comment in the code :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
