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:
> > > > > 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
> > 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?
>
> It's unfortunate that we don't have an existing test case, but I guess
> something like the following should demonstrate it's need:
> ```
> struct Foo {
> Foo(const int &);
> };
> void foo(Foo);
> void bar() {
> const int x = 0;
> foo(^x);
> }
> ```
>
> In the absence of that logic, we'd say `passed by reference` and not `by
> const reference`.
>
> > 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.
>
> I think this logic is setting up the right starting point for the case when
> we have implicit nodes in between. We should just make sure we bail out early
> when we don't have any implicit nodes in between and don't execute this
> logic. I agree that it just complicates things for that case.
Thanks for the test case! Since @kadircet supposes to keep it as-is, I'm happy
to follow the proposal. I'm going to restore the code change.
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
