[PATCH] D44954: [clangd] Add "member" symbols to the index

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

The change looks mostly good. Some nits and questions about the testing.



================
Comment at: clangd/index/Index.h:158
   unsigned References = 0;
-
+  /// Whether or not this is symbol is meant to be used for the global
+  /// completion.
----------------
s/this is symbol/this symbol/?


================
Comment at: clangd/index/SymbolCollector.cpp:158
+      translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+      enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+      objcCategoryImplDecl(), objcImplementationDecl()));
----------------
(Disclaimer: I don't know obj-c...)

It seems that some objc contexts here are good for global code completion. If we want to support objc symbols, it might also make sense to properly set `SupportGlobalCompletion` for them.


================
Comment at: clangd/index/SymbolCollector.cpp:359
+
+  // For global completion, we only want:
+  //   * symbols in namespaces or translation unit scopes (e.g. no class
----------------
Could we pull this into a helper function? Something like `S.SupportGlobalCompletion = DeclSupportsGlobalCompletion(...)`?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:657
 
+TEST(CompletionTest, DynamicIndexMultiFileMembers) {
+  MockFSProvider FS;
----------------
IIUC, this tests the `SupportGlobalCompletion` filtering logic for index completion? If so, I think it would be more straightforward to do something like:

```
Sym NoGlobal(...);
NoGlobal.SupportGlobalCompletion = false;
...
completions (Code, {NoGlobal, func(...), cls(...)})
// Expect only global symbols in completion results.
```

This avoid setting up the ClangdServer manually.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:741
 
+TEST(CompletionTest, Enums) {
+  EXPECT_THAT(completions(R"cpp(
----------------
It's not clear to me what the following tests (`Enums`, `AnonymousNamespace`, `InMainFile`) are testing. Do they test code completion or  symbol collector? If these test symbol collection, they should be moved int SymbolCollectorTest.cpp


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:785
+                  .items,
+              IsEmpty());
+
----------------
I think the behaviors above are the same before this change, so I'm not quite sure what they are testing. Note that symbols in main files are not collected into dynamic index, and by default the tests do not use dynamic index.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:846
+  auto Results = cantFail(runCodeComplete(Server, File, Test.point(), {}));
+  EXPECT_THAT(Results.items, IsEmpty());
+}
----------------
I think this is expected to be empty even for symbols that are not in anonymous namespace because symbols in main files are not indexed.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:70
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(SupportGlobalCompletion, SupportGlobalCompletion, "") {
+  return arg.SupportGlobalCompletion == SupportGlobalCompletion;
----------------
nit: I'd probably call this `Global` for convenience.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:216
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAreArray(
-                  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-                   QName("kStr"), QName("foo"), QName("foo::bar"),
-                   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-                   QName("foo::bar::v2"), QName("foo::baz")}));
+  EXPECT_THAT(Symbols, UnorderedElementsAreArray(
+                           {QName("Foo"),          QName("Foo::Foo"),
----------------
Could you also add checkers for the `SupportGlobalCompletion` field?


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:404
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-                                            QName("Green"), QName("Color2"),
-                                            QName("ns"), QName("ns::Black")));
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(QName("Red"), QName("Color"), QName("Green"),
----------------
Could you please also add checkers for `GlobalCodeCompletion` for these enums?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954