hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Thanks, this looks great! Please fix the code style of the unit-test (see my
other comment) before landing it.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1111
+ // Main file ranked higher than any #include'd file
+ break;
+
----------------
VitaNuo wrote:
> hokein wrote:
> > if we perform an early `return` inside the loop, the code can be simpler.
> >
> > ```
> > // Pickup the highest provider that satisfied the symbol usage, if
> > available.
> > // If the main-file is the chosen provider, we deliberately do not display
> > it (as it may cause too much noise, e.g. for local variables).
> > for (const auto & H : Headers) {
> > if (isMainFile(H)) {
> > return;
> > }
> > if (!Matches.empty()) {
> > HI.Provider = Matches[0]->quote();
> > return;
> > }
> > }
> >
> > HI.Provider = spellHeader(AST...);
> > ```
> Not really, this is not what we'd agreed upon.
>
> Consider the provider list `[foo.h, main-file]`, `foo.h` _not_ being
> #include'd.
> The above code will return without a provider, but we would actually like to
> return `foo.h`.
Yeah, you're right. I don't remember what I thought on writing the comment.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2894
+ const std::function<void(HoverInfo &)> ExpectedBuilder;
+ } Cases[] = {{Annotations(R"cpp(
+ struct Foo {};
----------------
Can you follow the the indentation-style of `R"cpp()cpp"` from the above
`TEST(Hover, NoHover)` example? It would be nice to be consistent.
We can save the explicit `Annotations` word here by changing the type of `Code`
from `Annotations` to `const char* const`, and constructing the `Annotations`
object inside the for-loop.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2938
+ )cpp"),
+ [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}};
+
----------------
VitaNuo wrote:
> hokein wrote:
> > I think this should be `""`, the using declaration is defined in main-file,
> > main-file should be the only one provider, and we don't display main-file
> > provider.
> This is the same situation as `absl::string_view` vs. `std::string_view`, I
> believe.
> The decl that is used in hover is `class Foo {};` from `foo.h`, so `foo.h` is
> correct AFAIU.
oh, right. I missed that fact that the target on Hover here is the underlying
decl of the using decl, can you add a comment clarifying the hover here is
showing the underlying 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
