hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
thanks, looks good from my side.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1175
+
+ for (const include_cleaner::Header &H : Providers) {
+ if (!ConvertedIncludes.match(H).empty()) {
----------------
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.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:62
#include <optional>
+#include <set>
#include <string>
----------------
nit: this is unused now.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1188
+ // in the main file.
+ return;
+ }
----------------
nit: my personal preference would be using `break` here.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3011
+ #include "bar.h"
+ #include "for^ward.h"
+ Bar *x;
----------------
it would be nice to have a comment explaining why we show nothing here (because
we have a higher provider `bar.h`)
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
