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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the review!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+  const SourceManager &SM = AST.getSourceManager();
+  llvm::SmallVector<include_cleaner::Header> Headers =
+      include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes());
----------------
hokein wrote:
> maybe name it `SortedProviders`, it contains the main-file (calling Headers is a bit confusing).
I think `RankedProviders` is better, they are not exactly sorted.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1111
+      // Main file ranked higher than any #include'd file
+      break;
+
----------------
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`.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2938
+  )cpp"),
+                [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}};
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144976/new/

https://reviews.llvm.org/D144976