hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1094
+void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
+ std::optional<const NamedDecl *> UsedDecl,
----------------
we can simplify the signature like `(ParsedAST&, include_cleaner::Symbol&,
HoverInfo&)`, constructing a `Symbol` in call site is trivial, and it can help
simplify the implementation (no sanity check for two `std::optional` etc).
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1138
+ if (H.kind() == include_cleaner::Header::Physical &&
+ H.physical() == SM.getFileEntryForID(SM.getMainFileID()))
+ continue;
----------------
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).
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+ trace::Span Tracer("Hover::maybeAddSymbolProviders");
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), MacroReferences, AST.getPragmaIncludes(),
----------------
VitaNuo wrote:
> hokein wrote:
> > It seems that the `walkUsed` API might be not the best fit. `walkUsed` API
> > has some special logic on handling different AST nodes, e.g. refs of
> > operators are ignored, so if we hover on an operator ref, we will not show
> > the providing header (which we should).
> >
> > Our goal is to provide the information (header) where the symbol under the
> > hover comes from (ref satisfaction is out of the scope). I think
> > `include_cleaner::headersForSymbol` is a better fit for our purpose, and
> > the implementation is simpler:
> >
> > - on hover, we have the selected symbol (either a regular declaration or a
> > macro)
> > - it is easy to construct a `include_cleaner::Symbol` from the selected
> > symbol
> > - choose the first result from `headersForSymbol`
> >
> > To do that we need to expose `headersForSymbol` from the internal
> > `AnalysisInternal.h`.
> Thank you! I am using `headersForSymbols` now.
>
> Ref satisfaction is not entirely out of scope.
> If the provider is included, we would like to show this provider in the hover
> card, irrespective of the ranking.
>
> If the provider is not included, we show the best provider from the whole
> list of possible providers.
>
> The behavior is different, based on ref satisfaction.
> Because of that, the implementation is actually not that much shorter than
> the version with `walkUsed`.
>
> However, you're right that it solves the issue with operators (wasn't aware
> of that, thanks!). I've added a test case for the hover on operators.
>
> As an additional bonus, it also solves the issue with `using` decls as
> discussed in a different comment thread below. We can now say `Provided by
> <string_view>` in the hover card that pops up for the `absl::string_view`
> references because we are doing the analysis on the `std::string_view` decl.
> Ref satisfaction is not entirely out of scope.
If the provider is included, we would like to show this provider in the hover
card, irrespective of the ranking.
Ah, right, I missed this point.
> Because of that, the implementation is actually not that much shorter than
> the version with walkUsed.
I think the implementation is still simpler, we don't need to non-trivial thing
like comparing ref range with the selected range, and ref symbol declaration vs
the selected symbol etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
