[PATCH] D41823: [clangd] Add more filters for collected symbols.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

LGTM. I'd add a comment pointing to code in include-fixer we're borrowing, though. (See the relevant comment)



================
Comment at: clangd/index/SymbolCollector.cpp:80
+                                    : decl(unless(isExpansionInMainFile())),
+                hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())))),
+            *D, *ASTCtx)
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > These ast matchers seems to be relatively simple, maybe we can use the `Decl` methods  to implement them at the moment. Or will we add more complex filters in the future?
> > I second this. Also the code would probably be more readable without matchers in this particular example.
> Yes, I expect to borrow more matchers from include-fixer's find-all-symbols. Also,  although `isExpansionInMainFile` seems easy to implement, I am inclined to reuse the existing code if possible.
Could you add a comment pointing to the code in include fixer, so that everyone reading the code knows where the code comes from?
(It's generally useful in case there are bugs and borrowing more code from include-fixer fixes that)


================
Comment at: clangd/index/SymbolCollector.h:28
+  struct Options {
+    bool IndexMainFiles = false;
+  };
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > We need documentation for the options.
> > Also, would naming it `OnlyMainFiles` make the intent of the option clearer?
> Actually, `OnlyMainFiles` is the opposite. We always index header files but optionally index main files. I've added comment to clarify...
The comment looks good, thanks!


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:107
   const std::string Header = R"(
-    class Foo {
-      void f();
-    };
+    class Foo {};
     void f1();
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Why do we remove `void f()`? Maybe assert that it's not in the list of symbols instead?
> I was trying to limit the scope of each test case (there is a separate test case `IgnoreClassMembers`). `UnorderedElementsAre` should be able to capture unexpected symbols.
Oh, sorry, I missed that the name of the test changed too. 
IMO, generally, adding new tests in seems like a better option if the original tests aren't broken.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:182
   )";
-  runSymbolCollector(/*Header=*/"", Main);
+  runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Why should we change this test?
> By default, main files are not indexed, so I put the code in header.
But we still get the symbols from main file in clangd, because we're asking to index main files in `indexAST`...
Makes sense. Thanks for clarifying.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41823