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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D41823#970725, @hokein wrote:

> With this patch, the index-based code completion will not provide any candidates for `ns1::MyClass::^` (as the index-based completion drops all  results from clang's completion engine), we need a way to combine symbols from 3 sources (static index, dynamic index and clang's completion engine).


Given that index-based completion is still experimental I think it's still fine to address this in a follow-up change. (In order to keep both changes simpler and more focused).



================
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;
----------------
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


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


================
Comment at: clangd/index/SymbolCollector.cpp:86
+    // FIXME: Should we include the internal linkage symbols?
+    if (!ND->hasExternalFormalLinkage() || ND->isInAnonymousNamespace())
+      return true;
----------------
This should probably include changes from @hokein 's patch D41759.


================
Comment at: clangd/index/SymbolCollector.h:28
+  struct Options {
+    bool IndexMainFiles = false;
+  };
----------------
hokein wrote:
> We need documentation for the options.
Also, would naming it `OnlyMainFiles` make the intent of the option clearer?


================
Comment at: unittests/clangd/FileIndexTests.cpp:173
   M.update(Ctx, "f1",
-           build("f1", "class X { static int m1; int m2;};").getPointer());
+           build("f1", "class X { int m2; static void f(); };").getPointer());
 
----------------
Why do we need to remove `static int m1`?
Perhaps simply adding `void f()` is enough?


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:107
   const std::string Header = R"(
-    class Foo {
-      void f();
-    };
+    class Foo {};
     void f1();
----------------
Why do we remove `void f()`? Maybe assert that it's not in the list of symbols instead?


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:182
   )";
-  runSymbolCollector(/*Header=*/"", Main);
+  runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(
----------------
Why should we change this test?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41823