[PATCH] D44954: [clangd] Add "member" symbols to the index

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44954#1104178, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D44954#1103664, @malaperle wrote:
>
> > It's probably better to consider this in a future patch. Maybe something like the first suggestion: vector::push_back and match both. Otherwise, I would think it might be a bit too verbose to have to spell out all of the specialization. Maybe we could allow it too. So... all of the above? :)
>
>
> This certainly does not have to be addressed in this patch. Just wanted to collect opinions on what the behavior we want in the long term.
>  My thoughts would be towards allowing only `vector::push_back` and make it match both `push_back`s: in `vector<bool>` and `vector<T>`.
>  Other cases might work too, but I wouldn't try implementing something that matches specializations. It's just too complicated in the general case. This will only be used in workspaceSymbol and it's fine to give a few more results there...


Sounds very reasonable.

>>> What scopes will non-scoped enum members have?
>> 
>> Hmm. I think all of them, since you can refer them like that in code too. Case #1 doesn't work but that was the case before this patch so it can probably be addressed separately. I'll add some tests though!
> 
> I would vote for making queries `En::A` and `A` match the enumerator, but **not** `::A`. The reasoning is: yes, you can reference it this way in a C++ file, but `workspaceSymbol` is not a real C++ resolve and I think it should match the outline of the code rather than the actual C++ lookup rules.
>  E.g. I wouldn't expect it to match symbols from base classes, etc. This should also simplify implementation too.

I don't have a strong opinion, so I can try this suggestion!



================
Comment at: clangd/index/Index.h:160
+  /// The Decl::Kind for the context of the symbol, i.e. what contains it.
+  Decl::Kind DeclContextKind;
+  /// Whether or not this is an enumerator inside a scoped enum (C++11).
----------------
ioeric wrote:
> malaperle wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > How do we use `DeclContextKind`?
> > > > Why did we decide to not go with a `bool ForCompletion` instead? (I'm probably missing a conversation in the workspaceSymbol review, could you point me to those instead?)
> > > > 
> > > > I'm asking because clang enums are very detailed and designed for use in the compiler, using them in the index seems to complicate things.
> > > > It feels we don't need this level of detail in the symbols. Similar to how we don't store the whole structural type, but rely on string representation of completion label instead.
> > > +1
> > > 
> > > `ForCompletion` sounds reasonable as the current design of index-based code completion relies on assumptions about contexts. 
> > My thinking was that the "ForCompletion" boolean was too specific and tailored for one client use. I thought the Symbol information should not have that much hard-coded knowledge on how it would be used. It would be odd to have "ForWorkspaceSymbol", "ForDocumentSymbol", etc. That is why I was going for a slightly more detailed symbol information that opened the door for more arbitrary queries for symbol clients. But it complicates things a bit more and I'd be happy to bring back the "ForCompletion" if it makes more sense for now.
> I think code completion, with the most complicated use of the index so far, probably deserves a flag :P I would expect/hope other features to be less "picky" about symbols. A high-level flag like `ForCompletion` would help keep knowledge about filtering for code completion (e.g. enums ...) inside symbol collector, which I think could be a win.
> 
> FWIW, `ForCompletion` makes it sound like a symbols is collected specifically for code completion. Maybe something like `bool SupportGlobalCompletion` would be better?
> Maybe something like bool SupportGlobalCompletion would be better?

Sounds good!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954