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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

LGTM (just a bunch of style comments from me), but I think you'll have to get someone else's attention if you're expecting to get signoff from someone to land this.

I also think it would make sense for you to land the absolutely trivial trailing-whitespace diffs right now without review, and then rebase this PR on top, so that those trivial whitespace diffs don't show up as distractions in this review. (In case it's useful to anyone reading: I use detab -R <http://www.club.cc.cmu.edu/~ajo/free-software/detab.c> to strip trailing spaces and convert hard tabs.)



================
Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:1907-1908
   constexpr X<S1> *p = const_cast<X<X<S1>>*>(&xxs1);
-  static_assert(p->f() == sizeof(X<S1>), ""); // expected-error {{constant expression}} expected-note {{virtual function}}
+  static_assert(p->f() == sizeof(X<S1>), ""); // cxx11-error {{constant expression}} cxx11-note {{virtual function}}
+  // cxx20_2b-error at -1 {{static_assert failed}}
 
----------------
I guess you're keeping the existing style here, but the existing style is not great.

Incidentally, I don't understand why Clang's error message is so dramatically different between C++11 and C++14 modes — in C++14 through 20 we get "static_assert failed" but in C++11 it wants specifically to talk about that virtual function call.
https://godbolt.org/z/1e8nqTbsb


================
Comment at: clang/test/SemaCXX/constant-expression-cxx14.cpp:230
   constexpr bool check(D &d) { return d.c.a.y == 3; }
+  // cxx20_2b-note at -1 {{read of member 'y' of union with active member 'x' is not allowed in a constant expression}}
 
----------------
Many of the existing snippets are IMHO too brief, but this new snippet was IMHO too verbose.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx14.cpp:292-295
+  static_assert(++ref(false), ""); // cxx14-warning {{deprecated}}
+  // cxx20_2b-error at -1 {{ISO C++17 does not allow incrementing expression of type bool}}
+  static_assert(++ref(true), ""); // cxx14-warning {{deprecated}}
+  // cxx20_2b-error at -1 {{ISO C++17 does not allow incrementing expression of type bool}}
----------------
Here's an example of "existing too brief" //plus// "new too verbose." The most important snippet of the diagnostic actually doesn't change between 14 and 20!


================
Comment at: clang/test/SemaCXX/conversion-function.cpp:144
   if (Cond)
-    return p; // expected-error{{calling a private constructor}}
-  
+    return p; // cxx98_14-error{{calling a private constructor}}
+
----------------
Oh this is a fun one. I almost want to ask for a code comment here, but I suppose anyone who breaks this test should be prepared to do the archeology anyway. :)

Prior to C++20 this didn't consider using `AutoPtr(AutoPtrRef)` to construct the return value, and so the first overload resolution (as rvalue) failed to find any candidates, and the second overload resolution (as lvalue) found the private ctor. Then in C++20 and later, we //do// consider `AutoPtr(AutoPtrRef)` during the first overload resolution, and it works fine, so the second overload resolution never happens.


================
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}}
----------------
When is this condition true?
(Honestly I'm surprised that the `expected-warning` parser understands `#if` in the first place.)


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


================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:144
+
+template_return_task<MoveOnly> param2template(MoveOnly value) {
+  co_return value; // We should deduce U = MoveOnly.
----------------
Nit: `return_template_task` or `generic_task` would look less confusingly like `template return_task...` at first glance. :)


================
Comment at: clang/test/SemaCXX/coroutines.cpp:1127
   CoroMemberTag test_qual(int *, const float &&, volatile void *volatile) const {
+    // cxx20_2b-warning at -1 {{volatile-qualified parameter type 'volatile void *volatile' is deprecated}}
     auto TC = co_yield 0;
----------------
Nit: Here and below, I'd be briefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99225