[PATCH] D146244: [clangd] Show used symbols on #include line hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, the implementation looks good. I left some comments around the test.



================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2984
 
+TEST(Hover, UsedSymbols) {
+  struct {
----------------
Can you add a test in the existing `TEST(Hover, Present)` to verify the `present()` method work as expected when the number provided symbols `<= 5`/ `> 5`?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2990
+                  #inclu^de "foo.h"            
+                  int var = foo();
+                )cpp",
----------------
can you add a macro `FOO` to the test?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2993
+                [](HoverInfo &HI) { HI.UsedSymbolNames = {"foo"}; }},
+               {"[[#incl^ude \"foo.h\"]]",
+                [](HoverInfo &HI) { HI.UsedSymbolNames = {}; }},
----------------
nit: remove the unused `[[]]`.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2996
+               {R"cpp(
+                  #include "foo.h"  
+                  #include ^"bar.h"
----------------
IIUC, this test is to test multiple symbols provided by a header, I would suggest removing the `foo.h`, and just keep `bar.h` (to reduce the noise from the test).





================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3000
+                  int barVar = bar();
+                  int foobarVar = foobar();
+                  X x;
----------------
It is not quite obvious to readers that `foobar`, and `X` is from the `bar.h`, maybe use some more obvious name (e.g. defining classes named `Bar1`, `Bar2` etc).


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3007
+               {R"cpp(
+                  #in^clude <foo>
+                  int fooVar = foo();
----------------
This test is to test the system header, I'd simulate it more further to reflect the reality. Let's use `<vector>`, and a `system/vector` file with the dummy content `namespace std { template<typename> class vector {}; }`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146244/new/

https://reviews.llvm.org/D146244