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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for sharing the example Marc! It's a bit surprising to see files that are not protobuf-generated named proto.h.

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

Pattern-based filtering could be an option as it wouldn't require code modification and could support potentially more filters, although I'm a bit worries about rules getting too complicated (e.g. filters on symbol kinds etc) or running into limitation.

But for now, it seems to me that the easiest approach is putting an option around proto heuristic for the symbol collector, until we need more filters.



================
Comment at: clangd/index/SymbolCollector.cpp:156
+  if (isPrivateProtoSymbol(ND, ASTCtx->getSourceManager()))
+    return true;
+
----------------
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.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:713
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+                                            QName("nx::Kind"),
----------------
ilya-biryukov wrote:
> Maybe also test that the same symbol is not excluded in the  file that does not end with `.proto.h`?
Sounds good.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751