[PATCH] D144976: [clangd] Add provider info on symbol hover.
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 16 09:20:01 PDT 2023
hokein added a comment.
Thanks, left some comments on the unittest, I think we can make it simpler.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1107
+ if (!Matches.empty()) {
+ // Pick the best-ranked included provider
+ Result = Matches[0]->quote();
----------------
The position of this comment is a bit confusing:
- `Headers` is sorted by the ranking
- `Matches` the result returned by `ConvertedIncludes.match` is not sorted.
The comment here reads like the `Matches` is sorted! I think it is better to move it to the outer for-loop. The first element of `Headers` which satisfies `!Matches.empty()` is the best-ranked provider.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1118
+
+ for (const auto &H : Headers) {
+ if (H.kind() == include_cleaner::Header::Physical &&
----------------
now the for-range loop doesn't seem necessary, we could always use the `Headers.front()`, right?
================
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>()) {
----------------
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.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1138
+ if (H.kind() == include_cleaner::Header::Physical &&
+ H.physical() == SM.getFileEntryForID(SM.getMainFileID()))
+ continue;
----------------
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`.
```
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:258
+std::vector<include_cleaner::SymbolReference>
+collectMacroReferences(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
----------------
let's keep it unchanged, we don't need any change for this function in this patch.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2897
+ const std::function<void(HoverInfo &)> ExpectedBuilder;
+ } Cases[] = {
+ {Annotations(
----------------
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"
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2901
+ struct Foo {};
+ Foo F = [[Fo^o]]{};
+ )cpp"),
----------------
we can git rid of the `int foo()` content in `foo.h` since this test only tests symbol refs that is satisfied by the main file.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2914
+ #include "foo.h"
+ int F = [[f^oo]]();
+ )cpp"),
----------------
I guest this test is testing we chose a right ranking header, but it is hard to see why foo.h is better than bar.h, since both foo.h and bar.h content are identical, it is becase foo.h is better matching the symbol name (to make it more obvious, we could make foo.h contains a definition, or add a comment explaining it).
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2922
+ #include "foo.h"
+ int F = [[^foo]]();
+ )cpp"),
----------------
unclear to me the intention of this testcase.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2948
+
+ Foo [[f^oo]] = 42;
+ )cpp"),
----------------
this case seems to be duplicated with the first one, can be removed, I think.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3008
+ "#define MACRO 5",
+ "#define MACRO 10", "",
+ [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }},
----------------
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.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3023
+ #include "foo.h"
+ int [[^F]] = MACRO(5);
+ )cpp"),
----------------
this seems to be duplicated with the first testcase, nothing related to macro, can be dropped as well.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3052
+ "", R"cpp(
+ // IWYU pragma: private, include "foo.h"
+ int foo();
----------------
I'd just drop the IWYU-related test, since this patch has nothing to do with IWYU (we have the IWYU-specific tests in include-cleaner unittest).
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3123
+
+ HoverInfo HIFooBar;
+ HIFooBar.Name = "foo";
----------------
What's the difference between `HIFoo` and `HIFooBar`? Looks like they are the same.
I guess you want to check the `<>` and `""` cases?
================
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");
----------------
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.
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
More information about the cfe-commits
mailing list