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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:525
 
+  if (const auto *Var = dyn_cast<VarDecl>(D)) {
+    // Check if we are inside CallExpr
----------------
kadircet wrote:
> umm, is this a fixme or a leftover? I suppose it is from a previous version of the patch.
Yep, previous version, sorry about that.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:713
+  }
+  if (I >= CE->getNumArgs())
+    return;
----------------
kadircet wrote:
> i don't think this is necessary, we are baling out if `FD->getNumParams() <= I` anyways.
It's possible for FD->getNumParams() to be higher than CE->getNumArgs(), for example due to default arguments. If we couldn't find selected node inside CallExpr, we shouldn't be trying to match it to some "random" argument in FunctionDecl.

It's a bit irrelevant now, after addressing other review comments ;-)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:716
+  // Extract matching argument from function declaration.
+  if (const FunctionDecl *FD = CE->getDirectCallee()) {
+    if (FD->isOverloadedOperator() || FD->isVariadic() ||
----------------
kadircet wrote:
> nit: put this before arg index finding logic and do an early exit:
> 
> ```
> if(!CE) return;
> auto *FD = CE->getDirectCallee();
> if(!FD || FD->isOverloaded || variadic..) return;
> ```
> 
> after doing this you can also change the arg index finding logic to:
> ```
> auto *SelectedArg = N->outerImplicit().ASTNode.get<Expr>();
> if(!SelectedParam) return; // you can also move this above other exit conditions.
> for (unsigned I = 0; I < min(CE->getNumArgs, FD->getNumParams())...) { // I suppose these two can be different in presence of default params ?
>   if (CE->getArg(I) == SelectedArg) {
>       // do magic;
>       break;
>   }
> }
> ``
Yea, that's nicer, thanks.

(I opted for "continue" rather than "break" to reduce indentation).


================
Comment at: clang-tools-extra/clangd/Hover.cpp:717
+  if (const FunctionDecl *FD = CE->getDirectCallee()) {
+    if (FD->isOverloadedOperator() || FD->isVariadic() ||
+        FD->getNumParams() <= I)
----------------
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


================
Comment at: clang-tools-extra/clangd/Hover.cpp:897
+    llvm::raw_string_ostream OS(Buffer);
+    OS << "Passed as " << *CalleeArgInfo;
+    Output.addParagraph().appendText(OS.str());
----------------
kadircet wrote:
> i bet there will always be some people getting confused no matter what we say in here (especially since we are not just printing the type, and I totally find printing param name useful especially in case of literals). But to align with other types of information we provide maybe say:
> 
> `Substitutes: ` ?
I am very much open to suggestions for phrasing, I'm not too happy about "Passed as". 

However, I am not a fan of Substitutes. If I saw that, I would have no idea what it's referring to. Since there is no visible connection between this hover card and the function call, without knowing what I'm looking at I would not make this connection. 

Might be just me. I am really as far from being a UI expert as possible. I'll ask around to see if people would get the Substitute: and get back to you on this one.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:728
+            C c;
+            c.fun([[^a]], b);
+          }
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2093
+
+Passed as int arg_a = 7)",
       }};
----------------
kadircet wrote:
> i find showing the default value a little bit confusing, as user is providing a different value. i can also see some value in it though, so up to you.
I did that for consistency with how we format function arguments. IMHO that's a good choice, but, as I already mentioned, I have no UI skills at all, so if you or someone else feels strongly about this, I won't argue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81169