rsmith added a comment.
In D88295#2365312 <https://reviews.llvm.org/D88295#2365312>, @Quuxplusone wrote:
> I believe the organization of that directory implies that the path should be
> `class/class.init/class.copy.elision/`, not `special/class.copy/`.
> Otherwise LGTM, and I like this new test better than the old one!
> I still can't help re actually landing this patch, as I don't have
> permissions either.
>
> EDIT: Actually I take that back; I don't understand why any existing test
> would have the path `special/class.copy/`, since no section by that name
> exists in the Standard. So, I defer to anybody with a strong opinion on the
> matter.
It's historical; we haven't reorganized our tests in response to the subclauses
in the standard being reorganized, and (for example) in C++98 the copy elision
wording was in subclause 12.8 [class.copy], within Clause 12 [special]. Moving
the test to `class/class.init/class.copy.elision/p1.cpp` would make sense to
me. Otherwise the change to copy elision looks good; the history here is long
and subtle, but the status quo after P1825R0 (which was moved as a DR) is clear
that volatile objects are not implicitly movable.
However, it looks like this change also affects users of `CES_AsIfByStdMove`,
and for "as if by `std::move`" semantics, I don't think `volatile` variables
should be ignored. This affects two things:
One is whether we produce the warning suggesting use of `std::move`. For
example, given:
X f() {
volatile X x;
return x;
}
... where `X` has a volatile copy constructor and a volatile move constructor,
I think we should produce the warning suggesting use of `std::move`.
The other is whether implicitly move in a `co_return` statement. In that case,
I think the use of `CES_AsIfByStdMove` is a mistake, and we should be using
`CES_Default` there.
So, how about we add another `CES` flag to indicate whether we should reject
variables with volatile-qualified type, and set it for `CES_Default` but not
for `CES_AsIfByStdMove`, and also change the `co_return` handling to use
`CES_Default`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88295/new/
https://reviews.llvm.org/D88295
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
