kadircet marked an inline comment as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:695
+ const PrintingPolicy &Policy) {
+ if (!N)
+ return;
----------------
nit: this is already checked before entering the function.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:871
+ if (CalleeArgInfo) {
+ Output.addRuler();
+ std::string Buffer;
----------------
I know you are planning to make more changes on how we render this, but let's
not forget to drop the ruler in here.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:717
+ if (const FunctionDecl *FD = CE->getDirectCallee()) {
+ if (FD->isOverloadedOperator() || FD->isVariadic() ||
+ FD->getNumParams() <= I)
----------------
adamcz wrote:
> kadircet wrote:
> > i can see the reason for variadics, but why bail out on overloaded
> > operators? also can we have some tests for these two?
> I could totally see us supporting variadic, I'm just not sure how useful that
> will be, so I didn't do it right away.
>
> As for the operators, I don't think we should trigger on those. There are two
> cases:
> - things that do not look like a function call: operator+, operator<<, etc.
> When you hover over a variable it's not immediately obvious what the "passed
> to" would be referring to. Something like:
> foo(a+b);
> if I see "a passed as const int&" I might think this is about passing it to
> foo() (at least of first glance).
> At the same time, the value of this is very low for operators. You know what
> their signature is already.
>
> -operator() - this is much more like a function call. Maybe it would make
> sense to support it. It gets tricky with matching CallExpr args to FD params
> though. I'll leave a todo for now and worry about this in a separate change,
> if don't mind.jj
ok makes sense, you are definitely right about nested case, I wasn't thinking
about it.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:728
+ C c;
+ c.fun([[^a]], b);
+ }
----------------
adamcz wrote:
> kadircet wrote:
> > can you also add a test for a literal ?
> Currently there is no hover at all for literals. The reason was that it
> wouldn't be useful. This information, however, might be, especially in cases
> like:
> printThis(that, true, false, true);
> knowing what these refer to could be useful.
>
> Do you think this is good enough reason to enable hover on literals, with
> just this information? I'm thinking yes, but I'm not sure.
you are right, i remember carving the codepath for handling literals but forgot
that we've disabled them at the time because there wasn't much info we could
surface. I believe this one is a pretty good candidate (assuming LSP doesn't
get "inline parameter annotations").
But definitely doesn't need to be handled in this patch, feel free to update
the fixme on `getHoverContents(Expr ...)` though, saying we might want to
surface callee information.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81169/new/
https://reviews.llvm.org/D81169
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
