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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
