[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 a comment.

thanks! that looks really useful, especially knowing when a parameter is being passed as a ref/val.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:292
 
+void fillParam(const ParmVarDecl *PVD, HoverInfo::Param &Out,
+               const PrintingPolicy &Policy) {
----------------
why not return a `HoverInfo::Param` instead ?


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


================
Comment at: clang-tools-extra/clangd/Hover.cpp:698
+// information about that argument.
+void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
+                           const PrintingPolicy &Policy) {
----------------
nit: move into anonymous namespace


================
Comment at: clang-tools-extra/clangd/Hover.cpp:700
+                           const PrintingPolicy &Policy) {
+  if (!N || !N->outerImplicit().Parent)
+    return;
----------------
nit: extract `N->outerImplicit()` to a variable, it seems to be repeated along.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:713
+  }
+  if (I >= CE->getNumArgs())
+    return;
----------------
i don't think this is necessary, we are baling out if `FD->getNumParams() <= I` anyways.


================
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() ||
----------------
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;
  }
}
``


================
Comment at: clang-tools-extra/clangd/Hover.cpp:717
+  if (const FunctionDecl *FD = CE->getDirectCallee()) {
+    if (FD->isOverloadedOperator() || FD->isVariadic() ||
+        FD->getNumParams() <= I)
----------------
i can see the reason for variadics, but why bail out on overloaded operators? also can we have some tests for these two?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:787
           HI->Value = printExprValue(N, AST.getASTContext());
+        maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
       } else if (const Expr *E = N->ASTNode.get<Expr>()) {
----------------
this place is getting crowded. maybe we should start passing `N` into `getHoverContents` and encapsulate the logic in there or something. no action needed, just thinking out loud.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:893
+
+  if (CalleeArgInfo) {
+    Output.addRuler();
----------------
let's put this after `Size` and before `Documentation` we've been keeping the documentation and definition as kind of a `footer` for a while now.
that would also drop the need for a ruler.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:897
+    llvm::raw_string_ostream OS(Buffer);
+    OS << "Passed as " << *CalleeArgInfo;
+    Output.addParagraph().appendText(OS.str());
----------------
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: ` ?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:728
+            C c;
+            c.fun([[^a]], b);
+          }
----------------
can you also add a test for a literal ?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2093
+
+Passed as int arg_a = 7)",
       }};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81169