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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1175
+
+        for (const include_cleaner::Header &H : Providers) {
+          if (!ConvertedIncludes.match(H).empty()) {
----------------
kadircet wrote:
> note that this will attribute a symbol to it's non-preferred providers too, e.g. if you have:
> h1.h:
> ```
> struct Foo; // defined in foo.h
> struct Bar; // defined in bar.h
> struct Baz; // defined in baz.h
> struct H1 {};
> ```
> 
> I believe we want the following:
> main.cc:
> ```
> #include foo.h // Provides Foo
> #include bar.h // Provides Bar
> #include h1.h // Provides Baz, H1
> 
> Foo *x;
> Bar *y;
> Baz *z;
> H1 *h;
> ```
> 
> and not saying that h1.h provides all Foo, Bar, Baz and H1 (otherwise an LLVM header will always provide dozens of symbols, e.g. https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Type.h#L109)
> 
> Basically in addition to checking that the particular header we're interested in being a provider, we should also be checking there were no other providers that are mentioned in the main file with a higher rank.
Yeah I know that and I've actually assumed until now that it's the correct behavior :) I.e., that we would like to report all the possible providers as providing the symbol, not only the highest ranked one. 

But what you're saying makes sense too. See the updated version. Also added a smaller version of the above as a test case.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1192
                                   const SymbolIndex *Index) {
   PrintingPolicy PP =
       getPrintingPolicy(AST.getASTContext().getPrintingPolicy());
----------------
kadircet wrote:
> can you introduce a trace::metric here for tracking hover distributions on certain symbol types? you can see an example in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/XRefs.cpp#L352
> 
> i believe the cases we care about are:
> - include
> - macro
> - keyword
> - expr
> - decl
> - attribute
> 
> (so that we have some idea about how often we provide this information)
Ok. This goes a bit out of scope, but I hope we won't get too bogged down in this. Have a look, I hope I understood the API correctly.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2996
+               {R"cpp(
+                  #include "foo.h"  
+                  #include ^"bar.h"
----------------
hokein wrote:
> 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).
> 
> 
> 
Ok, I will join it with the `foo.h` testcase then, because they've become the same thing essentially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244