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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
