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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/test/SemaCXX/implicitly-movable.cpp:14
+
+private:
+  A(const A &);
----------------
aaronpuchert wrote:
> Quuxplusone wrote:
> > aaronpuchert wrote:
> > > Is this testing what we want it to test? The private functions are just not part of the overload set, right?
> > > 
> > > I think you should make them public and `= delete` them.
> > > The private functions are just not part of the overload set, right?
> > 
> > Short answer "wrong." Overload resolution will make a candidate set consisting of //all// the signatures (even the private ones, even the deleted ones), and then pick the best match. If the best match is inaccessible (private), or if the best match is deleted, then it gives a hard error. So the idea of this test is to verify that inside `test_volatile`, Clang doesn't consider `A(volatile A&&)` to be the best match.
> > 
> > Making them public and deleted would neither help nor harm, in terms of correctness.
> > 
> > However, I agree with your above comment: it does seem like this test should actually run the code and verify that the correct overload is getting called. Right now it's unclear whether `test_volatile` is calling `A(const volatile A&)` as expected; or `A(A&&)`; or nothing at all. (Because copy elision is weird, I think we actually expect in this case that it is doing overload resolution to require that `A(const volatile A&)` is callable, but then eliding the copy anyway.)
> Nevermind, private functions are part of the overload set, and compilation fails on master, so this is fine.
I think a pure frontend test is fine here.

* We're not trying to test reference initialization, and I think we can just assume that initializing an `A&&` with a `volatile` doesn't work. But I also think we can drop the `test_normal` case entirely, because that's probably already covered, and with it the `A&&` overload.
* Judging from the change title it seems to me that copy elision is not the concern here, but whether there is an implicit move or not. (Certainly they are related, but copy elision has a separate test that seems to actually check the generated IR. If copy elision is a concern, another test should probably be added to `clang/test/CXX/special/class.copy/implicit-move-def.cpp`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88295