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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue May 15 09:56:19 PDT 2018


sammccall added a comment.

In https://reviews.llvm.org/D46751#1099633, @malaperle wrote:

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


I think a flag doesn't solve much of the problem, and adds new ones:

- users have to find the flag, and work out how to turn it on in their editor, and (other than embedders) few will bother. And each flag hurts the usability of all the other flags.
- if this heuristic is usable only sometimes, that's at codebase granularity, not user granularity. Flags don't work that way. (Static index currently has this problem...)
- these flags end up in config files, so if we later remove the flag we'll *completely* break clangd for such users

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

Is this still the case after the last revision (with the comment check?)
Agree we should only hardwire this on if we are confident that false positives are vanishingly small.



================
Comment at: clangd/index/SymbolCollector.cpp:101
+// we check whether it starts with PROTO_HEADER_COMMENT.
+bool isPrivateProtoSymbol(const NamedDecl &ND) {
+  const auto &SM = ND.getASTContext().getSourceManager();
----------------
We're going to end up calling this code on every decl/def we see.
Am I being paranoid by thinking we should check whether the file is a proto once, rather than doing a bunch of string matching every time?


================
Comment at: clangd/index/SymbolCollector.cpp:112
+
+  auto Name = ND.getName();
+  if (!Name.contains('_'))
----------------
this asserts if the name is not a simple identifier (Maybe operators or something will trigger this?).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751





More information about the cfe-commits mailing list