[PATCH] D99005: [clang] WIP: Implement P2266

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

Quuxplusone added a comment.

Conspicuously missing: tests for `decltype(auto)` return types, tests for `co_return`.

Personally I'd rather see these new p2266-related tests go in a separate test file, not appended to the existing file, so as to keep each test file relatively simpler and shorter. (But I don't know that those-in-charge would agree.)



================
Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:5
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected,cxx11_14_17_20,cxx11_14_17 %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected,cxx11_14_17_20,cxx11_14_17 %s
 
----------------
These labels seem to be getting unwieldy. I propose that you rename `cxx11_14_17` to `cxx17` and `cxx11_14_17_20` to `cxx17_20`.

(There are no cases where C++11, '14, '17's behavior differ.)

So then we'll have `cxx17`, `cxx17_20`, `cxx20`, `cxx20_2b`, and `cxx2b`.
IMHO it might even be nice to eliminate the overlapping labels; just, when a message is expected in two out of three modes, write two expectations.
```
  B1(B1 &&) = delete;
    // cxx20-note at -1 {{'B1' has been explicitly marked deleted here}}
    // cxx2b-note at -2 {{'B1' has been explicitly marked deleted here}}
```
What do you think? (This could also be severed into its own earlier commit.)


================
Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:325
+  MoveOnly&& rr = test_3a(static_cast<MoveOnly&&>(w)); // cxx11_14_17_20-note {{in instantiation of function template specialization}}
+}
+
----------------
FWIW here I would prefer
```
template MoveOnly& test_3a<MoveOnly&>(MoveOnly&);
template MoveOnly&& test_3a<MoveOnly>(MoveOnly&&);
```
with an expected error on the second line. The parameter-ness of `w` seems like a red herring in your version.


================
Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:335
+  try {
+    throw x; // cxx11_14_17_20-error {{call to implicitly-deleted copy constructor}}
+  } catch(...) {}
----------------
I believe this is incorrect; `throw x` here shouldn't implicit-move because `x`'s scope exceeds the nearest enclosing try-block.


================
Comment at: clang/test/CXX/special/class.copy/p33-0x.cpp:3
+// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -std=c++11 -fsyntax-only -verify=expected,cxx11 %s
+// cxx2b-no-diagnostics
 class X {
----------------
Could you please ensure that this test is updated to be tested in //all// modes? My interpretation of the old line 1 is it's been locked to run only in '11 mode (not '14 or '17 or '20), which is just bizarre. There's nothing '11-specific about this file that I can see. (And again that change can be severed from the rest of this PR and landed earlier, I hope.)


================
Comment at: clang/test/CXX/special/class.copy/p33-0x.cpp:41
         throw x2; // okay
-      throw x; // expected-error{{call to deleted constructor of 'PR10142::X'}}
+      throw x; // cxx11-error{{call to deleted constructor of 'PR10142::X'}}
     } catch (...) {
----------------
I believe this is incorrect; `throw x` here shouldn't implicit-move (not even in C++2b) because `x`'s scope exceeds the nearest enclosing try-block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99005