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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet marked an inline comment as done.
kadircet added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:929
+    if (CallPassType->PassBy != HoverInfo::PassType::Value) {
+      OS << "by "
+         << (CallPassType->PassBy == HoverInfo::PassType::ConstRef ? "const "
----------------
nit:
```
OS << "by ";
if(CallPassType->PassBy == ConstRef)
  OS << "const ";
OS << "reference ";
```


================
Comment at: clang-tools-extra/clangd/Hover.cpp:934
+    }
+    if (CalleeArgInfo->Name) {
+      OS << "as " << CalleeArgInfo->Name;
----------------
nit: again some redundant braces :D


================
Comment at: clang-tools-extra/clangd/Hover.cpp:760
+        break;
+      default:
+        PassType.Reference = false;
----------------
adamcz wrote:
> 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.
> 
> 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 would expect people to file bugs in such cases for missing calleearginfo on hover card, hence we catching up quickly to a big enough coverage (which I think is already provided by this baseline). But the same argument goes both ways, users will also likely file bugs when the passby info is wrong, only problem is they'll likely be confused for a while :D

> 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.

I agree, hence just dropping the passby field isn't enough, if we chose to go down this way I think we should drop full CalleArgInfo.


I don't think it is a decision we should block this patch on, we can decide what to do by looking at user reactions.


================
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.
----------------
adamcz wrote:
> 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?
yes totally! (actually I had figured this one out while reading the rest, but forgot to delete the comment, sorry)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81169