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