mizvekov added inline comments.
================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:41-45
struct MoveOnly {
- MoveOnly() {};
+ MoveOnly() = default;
MoveOnly(const MoveOnly&) = delete;
- MoveOnly(MoveOnly&&) noexcept {};
- ~MoveOnly() {};
+ MoveOnly(MoveOnly &&) = default;
+};
----------------
mizvekov wrote:
> Quuxplusone wrote:
> > This change makes `is_trivial_v<MoveOnly>` true, where before it was false.
> > I don't know whether this matters to the coroutine machinery. Ideally,
> > @aaronpuchert would weigh in; I don't know, and am just raising the issue
> > because it struck my attention.
> >
> > (If it //does// matter, then maybe we even want to add test coverage for
> > //both// the move-only-trivial case and the move-only-nontrivial case.)
> So these are the changes I picked from D68845.
>
> You were a reviewer, and you did not raise an objection there :)
>
> Though that is perfectly fine, that was a couple of years back and our
> perceptions just change I guess.
>
> Though once I have P2266 implementation rebased on top of this, I can see if
> this change still makes sense in our context, but I'd also like for
> @aaronpuchert to weight on this.
I think I see why this change was made. Both in @aaronpuchert 's patch and in
the P2266 implementation, we stop creating that (incorrect) temporary when
passing value between co_return expression and return_value. A non-trivial
value is a better test for the temporary, but it does not add anything to the
case with no temporary.
So what I am going to do, is to move just this change to the P2266
implementation patch.
================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:144
+
+template_return_task<MoveOnly> param2template(MoveOnly value) {
+ co_return value; // We should deduce U = MoveOnly.
----------------
mizvekov wrote:
> Quuxplusone wrote:
> > Nit: `return_template_task` or `generic_task` would look less confusingly
> > like `template return_task...` at first glance. :)
> Like previous, you did not raise an objection on the original D68845 :)
>
> But I agree with this change. But I'd like for @aaronpuchert to have a final
> say on this since these are his tests that I am merely taking, just so they
> do not sit there unused.
I will go ahead and make this change anyway, since he might be unavailable. We
can always revert it if he comes back and raises an objection, I guess :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99225/new/
https://reviews.llvm.org/D99225
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
