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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:994
       HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+      PassType.PassBy = getPassMode(PVD->getType());
+    }
----------------
v1nh1shungry wrote:
> 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?
i suppose you mean:

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

first of all the version of the patch that i propose doesn't involve any changes in behaviour here (as we actually have an implicit cast in between, and i am suggesting finding out passby based on type of the parmvardecl only when there are no casts in between).

i can see @nridge 's reasoning about indicating copies by saying pass by value vs ref, which unfortunately doesn't align with C++ semantics directly (as what we have here is a prvalue, and it is indeed passed by value, without any copies to the callee).

it isn't very obvious anywhere but the main functionality we wanted to provide to the developer was help them figure out if a function call can mutate a parameter they were passing in, therefore it didn't prioritise literals at all. we probably should've made better wording choices in the UI and talked about "immutability". hence from functionality point of view calling this pass by `value` vs `const ref` doesn't make a huge difference (but that's probably only in my mind and people are already using it to infer other things like whether we're going to trigger copies).

so i'd actually leave this case as-is, and think about what we're actually trying to provide by showing arg info on literals. as it's currently trying to overload the meaning of `passby` and causing confusions. since the initial intent was to just convey "immutability" one suggestion would be to just hide the `passby` information for literals.
otherwise from value categories point of view, these are always passed by value, but this is going to create confusion for people that are using it to infer "copies" and getting that right, while preserving the semantics around "is this mutable" just complicates things.

best thing moving forward would probably be to just have two separate fields, one indicating mutability and another indicating copies and not talking about pass by type at all.

---

sorry for the lengthy answer, LMK if it needs clarification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014