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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/test/SemaCXX/implicitly-movable.cpp:14
+
+private:
+  A(const A &);
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88295