[PATCH] D144976: [clangd] Add provider info on symbol hover.

[Haojian Wu via Phabricator via cfe-commits]

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