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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

> Here :)
>  http://www.sidefx.com/docs/hdk/_s_o_p___bone_capture_lines_8proto_8h_source.html

Didn't take along to find an example. Thanks for digging this up. That looks like a good enough reason to not apply proto-specific filtering based solely on the filename...

> I like the idea that things work "out of the box", we have to make sure that it doesn't make it buggy for certain code bases and impossible to work around.

Totally agree, we should only enable something that we all agree is good enough at detecting the frameworks that it never guesses wrong on real code.

> If handling for other libraries is added later it would be good to split out this code a bit. A collection of "filters" could be passed to symbol collector. Each filter/framework-handling could be in it's own source file. Later...

That LG, I guess we can iterate on the design in a CL, design doc or an email thread. However, it's outside the scope of this patch probably.

> I'm not a big fan of parsing file comment in proto. It seems a bit cumbersome and we might not be able (or too expensive) to do so for completion results from sema (if we do want to filter at completion time).

Why do you feel it's cubersome? Getting the first line of the file from SourceManager and looking at it seems easy.
I certainly don't see why this should be expensive if we do it at the right time (perhaps it means doing that when building the preamble and stashing the results alongside it, but that's also easy in our current setup).



================
Comment at: clangd/index/SymbolCollector.cpp:156
+  if (isPrivateProtoSymbol(ND, ASTCtx->getSourceManager()))
+    return true;
+
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > We should also run the same code to filter our those decls coming from sema completions.
> > Otherwise, they might still pop up in completion results if internal proto symbols were deserialized from preamble by clang before running the code completion.
> Just want to clarify before going further. IIUC, in *index-based* completion, the preamble could still contain symbols from headers such that sema completion could still give you symbols from headers. 
> 
> If we do need to build the filter into code completion, we would need more careful design as code completion code path is more latency sensitive.
For reference. As discussed outside this thread, we might have decls from headers in the sema completion items.
It does not seem too hard to add filtering for those as well: essentially, we just need to call the same function at code completion time.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751