[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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