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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks a lot! haven't checked the tests yet.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:716
+  for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
+    auto *Arg = CE->getArg(I);
+    if (Arg != OuterNode.ASTNode.get<Expr>())
----------------
nit: `Arg` seems to be used only once, consider inlining.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:721
+    // Extract matching argument from function declaration.
+    if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
+      HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, Policy));
----------------
nit: braces are usually omitted when "then" part has a single expression. (applies to multiple other places in the patch)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:726
+  }
+  if (!HI.CalleeArgInfo.hasValue())
+    return;
----------------
nit: you can drop `hasValue`, we mostly make use of the implicit bool conversion for checking optionals.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:735
+    if (E->getType().isConstQualified()) {
+      PassType.Const = true;
+    }
----------------
isn't this same as `PVD->getType().isConstQualified()`? maybe set it in there?

I think it is more explicit that way, because in theory the `Expr` above is always the `DeclRefExpr` referencing the current parameter, which is quite subtle to figure out while looking at this. Or am I missing something?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:760
+        break;
+      default:
+        PassType.Reference = false;
----------------
*sigh* wish we didn't have more than 60 types of implicit casts in c++.

I can see how it is hard to list everything in here and even if you did it would still be really hard to reason about them.
I can't help myself but wonder though, was there a reasoning when choosing what to leave out? Because that would help me a lot, and possibly future readers. (I guess I am also fine with "the list looked comprehensive enough", as we can always iterate over)

I am a little bit hesitant about the fail policy though(both in here and in other branches), as I can't prove to myself that being passed as a reference would be false in all the other cases for example. And showing incorrect information in here could really end up confusing the user. Unless there's a clear reasoning behind this failure mode, I would suggest dropping `CalleeArgInfo` completely to be on the safe side. That way we can improve the handling for important cases later on, without surfacing possibly wrong results. WDYT?


================
Comment at: clang-tools-extra/clangd/Hover.h:86
+    // implicit constructor call.
+    bool Reference = true;
+    // True if that reference is false. Never set for non-reference pass.
----------------
why is this true by default ?


================
Comment at: clang-tools-extra/clangd/Hover.h:87
+    bool Reference = true;
+    // True if that reference is false. Never set for non-reference pass.
+    bool Const = false;
----------------
s/false/const/ ?

what would you say to having an enum instead ?
```
enum PassBy {
  Value,
   Ref,
   ConstRef,
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81169