[PATCH] D81169: [clangd] Improve hover on arguments to function call

[Kadir Cetinkaya via Phabricator via cfe-commits]

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