[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/conversion-function.cpp:160
+  return "Hello"; // cxx98_14-error {{calling a private constructor}}
 #if __cplusplus <= 199711L
   // expected-warning at -2 {{an accessible copy constructor}}
----------------
mizvekov wrote:
> Quuxplusone wrote:
> > When is this condition true?
> > (Honestly I'm surprised that the `expected-warning` parser understands `#if` in the first place.)
> Too bad you can't also define macros for these expected declarations :)
>From my reading of the [[https://github.com/llvm/llvm-project/blob/1cc9d949a1233e8b17b3b345ccb67ca7296c1a6c/clang/lib/Frontend/InitPreprocessor.cpp#L399|code]], only when compiling this source as not C++ (__cplusplus undefined).

And you made me realize I made a mistake there.

So remember this is called from -cc1, and when you don't pass "-std=c++" option to it, it looks like it does not actually set __cplusplus.

So yeah I unintentionally removed some test coverage there, which I will restore, thanks for catching this!

I will also look for if I have made any similar mistakes in other parts of this patch.


================
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;
+};
----------------
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.


================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:144
+
+template_return_task<MoveOnly> param2template(MoveOnly value) {
+  co_return value; // We should deduce U = MoveOnly.
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99225/new/

https://reviews.llvm.org/D99225