Quuxplusone added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:4626
CES_AllowExceptionVariables = 4,
- CES_FormerDefault = (CES_AllowParameters),
- CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes),
- CES_AsIfByStdMove = (CES_AllowParameters | CES_AllowDifferentTypes |
- CES_AllowExceptionVariables),
+ CES_AllowRValueReferenceType = 8,
+ CES_ImplicitlyMovableCXX11CXX14CXX17 =
----------------
I believe `RValue` should be spelled `Rvalue`, throughout.
================
Comment at: clang/include/clang/Sema/Sema.h:4634
+ (CES_AllowParameters | CES_AllowDifferentTypes |
+ CES_AllowExceptionVariables | CES_AllowRValueReferenceType),
};
----------------
Unless I'm mistaken, `CES_AsIfByStdMove` is now a synonym for `CES_ImplicitlyMovableCXX20`. Could we discuss the pros and cons of simply removing `CES_AsIfByStdMove`? Is there some difference in connotation here, like maybe we are expecting them to diverge again in C++23 or beyond?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3141
FunctionDecl *FD = Step.Function.Function;
if (ConvertingConstructorsOnly) {
----------------
This loop is hard to understand before your patch. I don't think you made it any worse. But, consider whether you could pull out the condition so that this part became something like
FunctionDecl *FD = Step.Function.Function;
if (!IsSuitableConversionForImplicitMove(FD, NRVOCandidate, ConvertingConstructorsOnly)) {
break;
}
// Promote "AsRvalue" to the heap...
Actually, I don't even understand why `continue;` is being used in the old code. Doesn't that mean "skip this step and go on to the next step"?
================
Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:59
+ } catch (C c_move) {
+ throw c_move; // expected-error {{calling a private constructor of class 'test_throw_parameter::C'}}
+ }
----------------
(1) I would prefer to [additionally?] see a test case using `=delete`, rather than having the member exist but be private. We expect the same behavior in both cases, right? but `=delete` is more like the code we hope people are writing in practice.
(2) Is there any simpler way to write the "pre-C++20" and "C++20" versions of this test? It's ugly to repeat the entire class body in both branches of the `#if`, when the only difference is whether the "expected-note" is present or not.
================
Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:92
+ NeedRvalueRef() {}
+ ~NeedRvalueRef() {}
+ NeedRvalueRef(B &&);
----------------
In all of these tests, I don't see any reason to have `{}` when `;` would suffice; and I don't think you need the destructor to be explicitly declared at all; and in fact for `NeedRvalueRef` and `NeedValue`, I think just
struct NeedRvalueRef {
NeedRvalueRef(B&&);
};
would suffice, right?
================
Comment at: clang/test/SemaCXX/warn-return-std-move.cpp:87
// expected-note at -2{{to avoid copying}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
}
----------------
I would like to see a copy of this file, titled something like `warn-return-std-move-cxx20.cpp`, updated to run with `-std=c++20` instead of `-std=c++14`. (Initially I thought that the lines above indicated a flaw in your patch; it took me a while to realize that this test file is compiled only with `-std=c++14`. In C++20, we expect that `ConstructFromBase(Base&&)` will be called and the expected-warning will not be printed, right?)
I would also appreciate either seeing an actual test file with all the test cases from http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1155r2.html , or at least hearing (in reply to this comment) that you've tested them all and you believe their behavior is correct.
It looks to me as if every P1155r2 case is handled correctly **except** for this one:
struct To {
operator Widget() const &; // "copy"
operator Widget() &&; // "move"
};
Widget nine() {
To t;
return t; // C++17 copies; C++20 should move (but this patch still copies)
}
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88220/new/
https://reviews.llvm.org/D88220