[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:721
+    // Extract matching argument from function declaration.
+    if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
+      HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, Policy));
----------------
kadircet wrote:
> nit: braces are usually omitted when "then" part has a single expression. (applies to multiple other places in the patch)
Ah, yes, that. I keep doing that because I am personally strongly opposed to this convention. I respect the rules, of course, and avoid {} in those cases, but I think my personal feelings about it make it hard for me to avoid mistakes like this ;-). Sorry.

Fixed.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:735
+    if (E->getType().isConstQualified()) {
+      PassType.Const = true;
+    }
----------------
kadircet wrote:
> 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?
It's different. Consider:
class C {
 public:
  C(int &x) {}
};
void foo(const C &c) {}

int y;
foo(y);

PVD is const-qualified (and it's type is const C&), but y is int and passed by non-const reference to the C c-tor, which might mutate y, even though the resulting instance of C will be const.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:760
+        break;
+      default:
+        PassType.Reference = false;
----------------
kadircet wrote:
> *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?
I basically went through the list of all CK_ values and looked for ones that preserve the ref bit. There aren't many. Basically any type conversion other than casting to base must drop the ref bit (because it's a different type then) and then there's few that are explicitly about copying data. So in the end the default is to drop the reference bit and there's only a handful that preserve it (DerivedToBase, Decay and NoOp, basically).

I do agree with you that this is fragile, likely to be slightly wrong and too complicated. I can't think of a better solution though.

I prefer to not drop the whole CalleeArgInfo / hover thing in those cases because, even if you ignore the whole mutable/non-mutable bit of information, I still find the name of the argument it's passed as to be very valuable).

I like the idea of now showing the ref/const-ref/value bit in cases where we fear it might not be correct, but I do not know how to display this to the user without confusing them (i.e. if there's no mention of ref/const-ref, does that mean it's by-value or that we couldn't tell?). Note that I intentionally avoided showing the words "by value" to the user, since it will confuse everyone when used on pointers.

This is essentially why I was hesitant to extract the reference bit and why my initial change had only the type, to let user figure it out themselves. It's going to be quite hard to get this right.



================
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.
----------------
kadircet wrote:
> why is this true by default ?
Passing by reference is the default. It takes extra nodes in the AST to turn it to pass by value. Basically the defaults are what we see on the very inner node and then we adjust this as we go up the tree. Does that make sense?


================
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;
----------------
kadircet wrote:
> s/false/const/ ?
> 
> what would you say to having an enum instead ?
> ```
> enum PassBy {
>   Value,
>    Ref,
>    ConstRef,
> };
> ```
I initially had an enum for the whole thing, but the Ref/Const/Ref/Value is very independent from Converted, so it resulted in too many values and awkward code. A PassBy enum and a boolean for Converted bit is better.

Makes the code slightly and tests a lot more readable. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81169