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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D44954#1101922, @malaperle wrote:

> @ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make sense to add a flag to disable indexing members. Could you comment on that? What kind of granularity were you thinking? Would a "member" flag cover both class members (member vars and functions) and enum class enumerators for example? I think that would be reasonable. But I will also add symbols in main files too, so another flag for that? Hmmm.


Sam convinced me that members would still be interesting for our internal index service (e.g. locations of members would be useful for go-to-def). I'll investigate how much impact that would be by running the indexer with your change patched in, but I don't want to block you on that, so I'm fine with checking this in without any filter. We could revisit the filter design when needed.

We actually had an option for indexing symbols in main files :) But it was removed as it turned out to be unused for the features clangd had at that point. I think it would be reasonable to add it back if we start supporting collecting main file symbols again. Maybe out of the scope of this patch, but I am interested in the use cases you have for symbols in main files, besides in `workspaceSymbols`?



================
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).
----------------
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. 


================
Comment at: clangd/index/Index.h:162
+  /// Whether or not this is an enumerator inside a scoped enum (C++11).
+  bool InScopedEnum = false;
   /// A brief description of the symbol that can be displayed in the completion
----------------
In case we do need these fields, I think they should go into `index::SymbolInfo` (i.e. `SymInfo` above)? As Ilya said, `Symbol` might be the wrong level to put these. And we would want them in index-while-build anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954