[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

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

malaperle added a comment.

In https://reviews.llvm.org/D46751#1099235, @sammccall wrote:

> @malaperle to expand on what Eric said, adding proto hacks with false positives and no way to turn them off is indeed not the way to go here!
>  There's probably going to be other places we want to filter symbols too, and it should probably be extensible/customizable in some way.
>  We don't yet have enough examples to know what the structure should be (something regex based, a code-plugin system based on `Registry`, or something in between), thus the simplest/least invasive option for now (it's important for our internal rollout to have *some* mitigation in place).
>  @ioeric can you add a comment near the proto-filtering stuff indicating we should work out how to make this extensible?


I agree with all of that. What I don't quite understand is why a flag is not ok? Just a fail-safe switch in the mean time? You can even leave it on by default so your internal service is not affected. We know for a fact that some code bases like Houdini won't work with this, at least there will be an option to make it work.

In https://reviews.llvm.org/D46751#1099537, @ioeric wrote:

> In https://reviews.llvm.org/D46751#1099479, @malaperle wrote:
>
> > In https://reviews.llvm.org/D46751#1099097, @ioeric wrote:
> >
> > > > I think this is good if that's true that the comment is always there. I think it would be OK for this to be enabled by default, with a general option to turn heuristics off. Not sure what to call it... -use-symbol-filtering-heuristics :)
> > >
> > > We have other filters in the symbol collector that we think could improve user experience, and we don't provide options for those.
> >
> >
> > What others filters do you mean? If you mean skipping "members", symbols in main files, etc, I a working on making them not skipped, see https://reviews.llvm.org/D44954.
>
>
> I meant the filters in https://github.com/llvm-mirror/clang-tools-extra/blob/master/clangd/index/SymbolCollector.cpp#L93 e.g. filtering symbols in anonymous namespace, which we think should never appear in the index.


I'll be looking at adding them too. For workspaceSymbols it's useful to be able to find them and matches what we had before. But completion will not pull them from the index.

> I think members are more interesting than the private proto symbols. We want to un-filter members because there are features that would use them, so indexing them makes sense. But private proto symbols should never be shown to users (e.g. in code completion or workspaceSymbols).
> 
> I also think adding an option for indexing members would actually make more sense because they might significantly increase the index size, and it would be good to have options to disable it if users don't use members (e.g. including members can increase size of our internal global index service, and we are not sure if we are ready for that).

It sounds like we'll need both flags. We should discuss that because I'm planning to add even more (almost all?) symbols. I don't think it's common that users won't want members for workspaceSymbols though, but I see how this is not good for the internal indexing service.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751