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