[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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014