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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the reviews!



================
Comment at: clangd/index/FileIndex.cpp:22
                                      llvm::ArrayRef<const Decl *> Decls) {
-  auto Collector = std::make_shared<SymbolCollector>();
+  auto CollectorOpts = SymbolCollector::Options();
+  CollectorOpts.IndexMainFiles = true;
----------------
ilya-biryukov wrote:
> NIT: why not `SymbolCollector::Options CollectorOpts`? It's seems more concise.
> 
> Or even `SymbolCollector::Options{/*IndexMainFiles=*/true}`; 
> The last one does not play nicely with moving the fields of the struct around, though
I think I had too much shared/unique pointers... :P

> The last one does not play nicely with moving the fields of the struct around, though
Yeah, this is the reason I didn't use initializer.


================
Comment at: clangd/index/SymbolCollector.cpp:80
+                                    : decl(unless(isExpansionInMainFile())),
+                hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())))),
+            *D, *ASTCtx)
----------------
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.


================
Comment at: clangd/index/SymbolCollector.h:28
+  struct Options {
+    bool IndexMainFiles = false;
+  };
----------------
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...


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:107
   const std::string Header = R"(
-    class Foo {
-      void f();
-    };
+    class Foo {};
     void f1();
----------------
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.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:182
   )";
-  runSymbolCollector(/*Header=*/"", Main);
+  runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(
----------------
ilya-biryukov wrote:
> Why should we change this test?
By default, main files are not indexed, so I put the code in header.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41823