v1nh1shungry added inline comments.
================
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:
> > > 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.
> hence from functionality point of view calling this pass by value vs const
> ref doesn't make a huge difference
Agree. But since we now choose to provide information in such a way, we should
keep it as correct as we can. But for this case, I'm good with both two (indeed
I now prefer `value` because of the prvalue, I was confused again :/ )
However, when I try to follow your proposal, I find the current code is somehow
complicated for me to understand. You said we can get rid of `if (isLiteral(E)
&& N->Parent == OuterNode.Parent)`, that's right, but how about the rest?
```
if (const auto *E = N->ASTNode.get<Expr>()) {
if (E->getType().isConstQualified())
PassType.PassBy = HoverInfo::PassType::ConstRef;
}
```
Whether it exists the tests won't break. If we remove it, the `CK_NoOp` cases
seem to rely on the default value of `HoverInfo::PassType` when there's an
implicit cast. If we keep this, why we're saying if the expression under the
cursor is const-qualified then the `PassType.PassBy` will be `ConstRef`? What
about `const int`?
So I think if we want to distinguish the cast case and the non-cast case, it
needs a better starting point for the cast case. Or we get the
`PassType::PassBy` through the parameter's type, which seems to be more
intuitive, and then do the correction according to the cast kind if we'd like.
I think it's kind of the same as the original implementation. They both are
based on the cast kinds.
> 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
Remove the code inside the branch and it'll behave as the same as the original
implementation.
> 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.
+1
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142014/new/
https://reviews.llvm.org/D142014
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
