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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
