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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1175
+
+        for (const include_cleaner::Header &H : Providers) {
+          if (!ConvertedIncludes.match(H).empty()) {
----------------
hokein wrote:
> VitaNuo wrote:
> > 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.
> thanks for raising it (I didn't think too much about this case). 
> 
> > 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)
> 
> Agree that for this case, we should prevent providing dozens of forward-declared symbols.
> 
> But the combination behavior of the unused diagnotics and hover can be confusing for case where `h1.h` provides all forward declarations and it is not the highest provider in any symbol refs of main.cc, so we will:
> 1) `h1.h` is considered as used -> no unused-include diagnostic 
> 2) `hover` on `h1.h` doesn't show any provided symbols
> 
> My mental model of 2) is that it indicates `h1.h` is not used, which seems to conflict with 1).  Maybe we can think it as a feature, for this case, it is safe to remove this header.
No solution here, just a comment: this seems to go along the lines of our discussion in the sync today, i.e., that we might want to make the unused include analysis stricter eventually. So it seems like the general tendency might be towards making the unused include analysis stricter, rather than showing more provided symbols on hover.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244