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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
