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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

(pardon the interruption, some drive-by comments :))



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1174
+          return;
+
+        for (const include_cleaner::Header &H : Providers) {
----------------
note that we don't care about each reference of a target, so speed things up (rather than going through each reference inside the main file), you can check if you've seen Ref.Target before and skip processing providers in that case.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1175
+
+        for (const include_cleaner::Header &H : Providers) {
+          if (!ConvertedIncludes.match(H).empty()) {
----------------
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.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1192
                                   const SymbolIndex *Index) {
   PrintingPolicy PP =
       getPrintingPolicy(AST.getASTContext().getPrintingPolicy());
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244