[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

In D142014#4084263 <https://reviews.llvm.org/D142014#4084263>, @nridge wrote:

> In D142014#4081922 <https://reviews.llvm.org/D142014#4081922>, @v1nh1shungry wrote:
>
>> I just came up with a case where the original implementation and this patch behave differently.
>>
>>   void foobar(const float &);
>>   int main() {
>>     foobar(0);
>>            ^
>>   }
>>
>> Used to `Passed by value`, now it is `Passed by const reference`. I think the former one is apparently better.
>
> Why is "passed by value" better?
>
> My mental model here is that the value does not get copied; for a built-in type that may not be much of a distinction, but if you imagine generalizing to a class type constructed using a user-defined literal, it could be a noncopyable (and in C++17, even non-moveable) type that can't be passed around by value.

Ah, I think you're right. I was confused. This case seems good to me. I guess I was thinking about

  void foobar(const float &);
  int main() {
    int a;
    foobar(a);
           ^
  }

Used to be `Passed by value`, now it is `Passed by const reference`. I think it's kind of confusing because the local variable `a` isn't actually referenced, right?



================
Comment at: clang-tools-extra/clangd/Hover.cpp:994
       HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+      PassType.PassBy = getPassMode(PVD->getType());
+    }
----------------
kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > sorry for showing up late to the party but instead of changing rest of the code, can we apply this logic only when there are no implicit casts/conversions between the arg and callexpr (i.e `N == &OuterNode`)?
> > To make sure I understand it correctly, do you mean I should give up any other code changes I made but keep this logic, and put this logic into the `N == &OuterNode` branch?
> > 
> > Sorry if I misunderstood anything! Shame for not being a good reader :(
> > To make sure I understand it correctly, do you mean I should give up any other code changes I made but keep this logic, and put this logic into the N == &OuterNode branch?
> 
> Somewhat.
> 
> Basically this code had the assumption that if we don't see any casts/conversions between the expression creating the argument and the expression getting passed to the callexpr, it must be passed by reference, and this was wrong. Even before the patch that added handling for literals.
> 
> The rest of the handling for casts/conversions/constructors in between have been working so far and was constructed based on what each particular cast does, not for specific cases hence they're easier (for the lack of a better word) to reason about. Hence I'd rather keep them as is, especially the changes in handling `MaterializeTemporaryExpr` don't sound right. I can see the example you've at hand, but we shouldn't be producing "converted" results for it anyway (the AST should have a NoOp implicit cast to `const int` and then a `MaterializeTemporaryExpr`, which shouldn't generated any converted signals with the existing code already).
> 
> Hence the my proposal is getting rid of the assumption around "if we don't see any casts/conversions between the expression creating the argument and the expression getting passed to the callexpr, it must be passed by reference", and use the type information in `ParmVarDecl` directly when we don't have any implicit nodes in between to infer `PassBy`.
> This should imply also getting rid of the special case for literals (`if (isLiteral(E) && N->Parent == OuterNode.Parent)`).
> 
> Does that make sense?
Thanks for the detailed explanation! But before we go ahead here, what do you think about the new test case I'm talking about above? Do you agree with my conclusion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014