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

Sun Mar 21 13:38:16 PDT 2021

mizvekov added a comment.

In D99005#2639977 <https://reviews.llvm.org/D99005#2639977>, @Quuxplusone wrote:

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

Though the first comment warns you that this is a work in progress and I just didn't get around to doing it yet ;)

Though `decltype` I had tested manually, co_return I have just begun testing at all.

I believe the `decltype` tests should go into the other file relevant to the pertinent section. One problem with leaving them here is that it would be extra noise for the tests for the older standards which don't even support the keyword at all.

> 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.)

They go into files named after section / paragraph, and I haven't come across examples where they were split yet, but I haven't looked specifically for this.

In D99005#2640121 <https://reviews.llvm.org/D99005#2640121>, @aaronpuchert wrote:

> With my previous comment I meant that it's better if you leave out the `co_return` bits for now because it's wrong anyway. We can't use `PerformMoveOrCopyInitialization`. It would just generate merge conflicts.

I am using `PerformMoveOrCopyInitialization` with AllowNRVO always false for co_return, which effectively makes it a `PerformCopyInitialization` (just replacing them instead was one of the WIP cleanups I mentioned).
I am new with this part of the code here, so maybe this is obvious, but why is it broken? Do you have any examples of test cases where this would fail?
Would this patch here cause any regressions?
Right now this is passing the test suite, and seems to be reporting the correct results for the tests you created for D68845 <https://reviews.llvm.org/D68845>.
Remember, here we just want to evaluate the solution proposed for P2266 <https://reviews.llvm.org/P2266> by testing this at large, in order to build support for it to be accepted into the standard.
Just waiting for it to be accepted first before implementing it would be too late :)

================
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

----------------
Quuxplusone wrote:
> 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.)
I like the idea of shortening the labels. Duplicating the warnings if it can be avoided easily, not so much. DRY is my mantra :D

Though what you said about severing this into an earlier commit, I think something may need to be done about it anyway.
So right now the tests are all over the place about which standard they are running, and none are testing c++2b at all...
I think we might need to gather input on this. Just changing every test to run every standard might be too much, so we could need a less obvious criteria.

================
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}}
+}
+
----------------
Quuxplusone wrote:
> 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.
Sure, sounds good.

================
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(...) {}
----------------
Quuxplusone wrote:
> I believe this is incorrect; `throw x` here shouldn't implicit-move because `x`'s scope exceeds the nearest enclosing try-block.
Err, you are right, my bad, this is an easy fix...

================
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 {
----------------
Quuxplusone wrote:
> 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.)
See my earlier comment about doing this systematically.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99005