Page MenuHomePhabricator

[libcxx] LWG2442: call_once() shouldn't DECAY_COPY()
ClosedPublic

Authored by K-ballo on Jun 2 2015, 11:16 AM.

Details

Summary

Implement LWG2442, use references to call_once arguments instead of decay-copying them.

Diff Detail

Event Timeline

K-ballo updated this revision to Diff 26989.Jun 2 2015, 11:16 AM
K-ballo retitled this revision from to [libcxx] LWG2442: call_once() shouldn't DECAY_COPY().
K-ballo updated this object.
K-ballo edited the test plan for this revision. (Show Details)
K-ballo added reviewers: mclow.lists, EricWF.
K-ballo added a subscriber: Unknown Object (MLST).
EricWF added inline comments.Jun 4 2015, 12:05 PM
include/mutex
445

This change is being made in order to support non-copyable Callable objects, right? I'm not convinced that always adding const is the correct thing to do. Without the const the new signature won't accept rvalues as input but adding const requires Callable to be const-callable.

Why not provide two signatures? It's not perfect but I think it is better.

void call_once(once_flag&, _Callable&); // accepts non-const lvalues.
void call_once(once_flag&, _Callable const&); // accepts rvalues and const lvalues.

Thoughts?

test/std/thread/thread.mutex/thread.once/thread.once.callonce/call_once.pass.cpp
160

That seems like it might indicate a problem. Do you know what is going on here? Are you sure you want to work around this?

236

Could you also add tests for the following:

  1. We don't copy the function.
  2. We accept a temporary as a function.
  3. If given a Function object with operator()(...) & and operator()(...) && do we call the correct one depending on the value category of the given functor?
K-ballo added inline comments.Jun 4 2015, 12:41 PM
include/mutex
445

I initially considered adding a perfectly forwarding non-variadic version when rvalue-refs are supported, and the two overloads you suggest above otherwise. This would add quite a lot of noise (each signature is present thrice). I guestimated const-ref to be enough, but I will add as many overloads and workarounds as you deem necessary.

test/std/thread/thread.mutex/thread.once/thread.once.callonce/call_once.pass.cpp
160

If you expand the context a bit you will see that I took it from MoveOnly right above, I have not experienced the issue myself.

236

Could you also add tests for the following:
We don't copy the function.
We accept a temporary as a function.

This is already the case, NonCopyable() is the function.

If given a Function object with operator()(...) & and operator()(...) && do we call the correct one depending on the value category of the given functor?

Good idea, will do.

K-ballo updated this revision to Diff 27341.Jun 8 2015, 3:28 PM

Addressed review comments:

  • Added a non-const lvalue ref overload for the non-variadic case
  • Added test case using ref-qualifiers
EricWF accepted this revision.Jun 12 2015, 8:48 AM
EricWF edited edge metadata.

LGTM and thanks! I'm going to cleanup the original call_once tests and add new tests once this is committed.

@mclow.lists We want to adopt this change retroactively, correct?
@K-ballo Do you want me to commit this for you?

This revision is now accepted and ready to land.Jun 12 2015, 8:48 AM

@K-ballo Do you want me to commit this for you?

Sure, go ahead.

mclow.lists accepted this revision.Jun 12 2015, 4:45 PM
mclow.lists edited edge metadata.

A nit, but other than that, LGTM.

test/std/thread/thread.mutex/thread.once/thread.once.callonce/call_once.pass.cpp
242

Let's use the TEST_STD_VERS macro instead of the naked __cplusplus here

EricWF closed this revision.Jun 12 2015, 7:27 PM

Committed as r239654. Thanks.