VitaNuo added a comment.
Thanks for the comments and the discussions!
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1118
+
+ for (const auto &H : Headers) {
+ if (H.kind() == include_cleaner::Header::Physical &&
----------------
hokein wrote:
> now the for-range loop doesn't seem necessary, we could always use the
> `Headers.front()`, right?
Ok. It seems like I need to put an `empty` check on `Headers`, however, because
contrary to the loop, `front()` might actually crash AFAIU.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1219
maybeAddCalleeArgInfo(N, *HI, PP);
+ maybeAddSymbolProviders(AST, *HI, include_cleaner::Symbol{*DeclToUse});
} else if (const Expr *E = N->ASTNode.get<Expr>()) {
----------------
hokein wrote:
> I wonder how well it works for the `NamespaceDecl`. NamespaceDecl is usually
> declared in many files, we will basically show a random provider in hover.
> We're not interested in `NamesapceDecl`, we probably need to ignore it.
>
> No action required here, but this is something we should bear in mind.
Thank you.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1138
+ if (H.kind() == include_cleaner::Header::Physical &&
+ H.physical() == SM.getFileEntryForID(SM.getMainFileID()))
+ continue;
----------------
hokein wrote:
> VitaNuo wrote:
> > hokein wrote:
> > > MainFile provider is a special case (I don't recall the details).
> > >
> > > IIUC, the model is:
> > >
> > > 1) a symbol usage that is satisfied (e.g. any of its providers that are
> > > directly included in the main file), we show the one with highest rank of
> > > these included providers
> > > 2) a symbol usage that is not satisfied (we choose the highest rank of
> > > all providers)
> > > 3) If the provider is the main-file, we don't show it in the hover card.
> > >
> > > Based on 1), if the main-file provider is the highest, we will not show
> > > it in the hover based on 3). However, the current implementation doesn't
> > > match this behavior
> > > -- on L1123 `ConvertedIncludes.match(H)` is always false if H is a
> > > main-file, and we will choose a lower-rank provider if the main-file is
> > > the first element of `Headers`
> > > -- the logic here doesn't seem to work, we should do a `break` on L1139
> > > rather than `continue`, which means we always use the `Headers[0]`
> > > element.
> > >
> > > Not sure we have discussed 3), one alternative is to show the information
> > > for main-file provider as well, it seems fine to me that the hover shows
> > > `provided by the current file` text (not the full path).
> > > we should do a break on L1139 rather than continue
> >
> > Ok. I'm not sure if this is of great practical importance (what are the
> > chances that the main file is the first provider, and there are more
> > providers for the same symbol?),
> > but you're right that we shouldn't show the lower-ranked provider for this
> > case.
> >
> > > one alternative is to show the information for main-file provider as well
> >
> > Yeah, this is possible ofc, but I'm not sure what the use would be. The
> > general intention of this feature (or feature set, even) is to help users
> > eliminate unnecessary dependencies, and they can hardly get rid of the main
> > file :)
> > So of the two options, I'd rather opt for not showing anything at all.
> > I'm not sure if this is of great practical importance
> I think the code should match our mental model, otherwise it is hard to
> reason about whether the actual behavior is expected or a bug.
>
> > (what are the chances that the main file is the first provider, and there
> > are more providers for the same symbol?),
>
> I think the pattern is not rare (especially for headers), think of the case
> below.
>
> ```
> #include "other.h" // other.h transitively includes the `foo.h`
>
> class Foo;
> const Foo& createFoo();
> ```
>
> although the main-file provider is not the top1 given the current ranking
> implementation, we have a plan to address it, see the FIXME in
> https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp#L219.
> After addressing the FIXME, the main-file could be the top1.
>
> > on L1123 ConvertedIncludes.match(H) is always false if H is a main-file,
> > and we will choose a lower-rank provider if the main-file is the first
> > element of Headers
>
> This comment doesn't seem to be addressed, now it is L1105.
>
> Given the following case, if the returned providers is [main-file, `foo.h`],
> the current code will show `foo.h` in hover.
> However, based on our mental model:
> 1) the main-file is one of the `Foo` providers, and it is the top1, we
> choose it
> 2) we don't show main-file provider in hover
> -> we should not show any information in hover.
>
> ```
> #include "foo.h"
>
> class Foo;
> Foo* b; // hover on `Foo`.
> ```
> I think the pattern is not rare (especially for headers), think of the case
> below.
Makes sense, I didn't think about headers at all.
> This comment doesn't seem to be addressed, now it is L1105.
Oops. See the current version please.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:258
+std::vector<include_cleaner::SymbolReference>
+collectMacroReferences(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
----------------
hokein wrote:
> let's keep it unchanged, we don't need any change for this function in this
> patch.
oh it's not changed, just got moved because of the other two. But no worries, I
can put it exactly in the previous position.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2897
+ const std::function<void(HoverInfo &)> ExpectedBuilder;
+ } Cases[] = {
+ {Annotations(
----------------
hokein wrote:
> I have some trouble following the test here:
>
> - this is a long list of testcases (see my other comments)
> - we construct each testcase with five fields, it is hard to track which
> content is for foo.h/bar.h/foobar.h
>
> I'd suggest to find way to simplify it, some ideas
> - we can use a fixed content foo.h, foo_fwd.h for all testcases (I guess
> something like below should be enough to cover the major cases), so that we
> don't need to specify all these content in every testcase.
> - only test critical cases 1) symbol ref is satisfied (by the main-file
> provider or by regular headers) 2) symbol ref is not satisfied, verify we
> show first provider of `Providers`. And test 3 different symbol kinds
> (regular decl, macro, operator)
>
>
> // foo.h
> #define FOO 1
> class Foo {};
> Foo& operator+(const Foo, const Foo);
>
> // foo_fwd.h
> class Foo;
>
> // all.h
> #include "foo.h"
> #include "foo_fwd.h"
Ok, I have simplified the test considerably. Please take a look.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2922
+ #include "foo.h"
+ int F = [[^foo]]();
+ )cpp"),
----------------
hokein wrote:
> unclear to me the intention of this testcase.
Testing that `foo.h` is ranked higher since it's providing the symbol directly.
But maybe this makes no sense, I will remove it then.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2948
+
+ Foo [[f^oo]] = 42;
+ )cpp"),
----------------
hokein wrote:
> this case seems to be duplicated with the first one, can be removed, I think.
yeah this is also provided by the main file
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3008
+ "#define MACRO 5",
+ "#define MACRO 10", "",
+ [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }},
----------------
hokein wrote:
> This will have macro-redefined warnings, `foo.h` is better than `bar.h`
> because the MACRO in main-file refers to the one defined in `foo.h`. Not sure
> the value of this testcase, I would suggest dropping it for simplicity.
>
> The same for the below one.
ok.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3023
+ #include "foo.h"
+ int [[^F]] = MACRO(5);
+ )cpp"),
----------------
hokein wrote:
> this seems to be duplicated with the first testcase, nothing related to
> macro, can be dropped as well.
ok.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3123
+
+ HoverInfo HIFooBar;
+ HIFooBar.Name = "foo";
----------------
hokein wrote:
> What's the difference between `HIFoo` and `HIFooBar`? Looks like they are the
> same.
>
> I guess you want to check the `<>` and `""` cases?
yes, thank you.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3139
+ #include "absl_string_view.h"
+ absl::str^ing_view abc; // hover on string_view
+ )cpp");
----------------
hokein wrote:
> I think we can simplify this test and merge it to the above `TEST(Hover,
> Providers)`.
>
> Just verifying the hover symbol respects the using decl is sufficient,
> something like
>
> ```
> #include "foo.h"
>
> namespace ns {
> using ::Foo;
> }
>
> ns::F^oo d; // verify that provider in hover is the main-file.
> ```
Ok, I will translate this to a "foobar" example. But I think the correct answer
in this example is actually `foo.h` since we want the provider of the
underlying decl, not the using decl.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
