[PATCH] D99225: [clang] tests: cleanup, update and add some new ones

[Matheus Izvekov via Phabricator via cfe-commits]

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