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

[K-ballo]

Summary

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

[EricWF]

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?

224

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]

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.

224

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]

Addressed review comments:

• Added a non-const lvalue ref overload for the non-variadic case
• Added test case using ref-qualifiers

[EricWF]

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?

[K-ballo]

In D10191#187322, @EricWF wrote:

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

Sure, go ahead.

[mclow.lists]

A nit, but other than that, LGTM.

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

230

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

[EricWF]

Committed as r239654. Thanks.

include/mutex

 template<class _Callable, class... _Args>
 _LIBCPP_INLINE_VISIBILITY
 void call_once(once_flag&, _Callable&&, _Args&&...);
 
 #else  // _LIBCPP_HAS_NO_VARIADICS
 
 template<class _Callable>
 _LIBCPP_INLINE_VISIBILITY
-void call_once(once_flag&, _Callable);
+void call_once(once_flag&, const _Callable&);
 
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 struct _LIBCPP_TYPE_VIS_ONLY once_flag
 {
     _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR
         once_flag() _NOEXCEPT : __state_(0) {}
 
 private:
     once_flag(const once_flag&); // = delete;
     once_flag& operator=(const once_flag&); // = delete;
 
     unsigned long __state_;
 
 #ifndef _LIBCPP_HAS_NO_VARIADICS
     template<class _Callable, class... _Args>
     friend
     void call_once(once_flag&, _Callable&&, _Args&&...);
 #else  // _LIBCPP_HAS_NO_VARIADICS
     template<class _Callable>
     friend
-    void call_once(once_flag&, _Callable);
+    void call_once(once_flag&, const _Callable&);
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 };
 
 #ifndef _LIBCPP_HAS_NO_VARIADICS
 
 template <class _Fp>
 class __call_once_param
 {
-    _Fp __f_;
+    _Fp& __f_;
 public:
-#ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
     _LIBCPP_INLINE_VISIBILITY
-    explicit __call_once_param(_Fp&& __f) : __f_(_VSTD::move(__f)) {}
-#else
-    _LIBCPP_INLINE_VISIBILITY
-    explicit __call_once_param(const _Fp& __f) : __f_(__f) {}
-#endif
+    explicit __call_once_param(_Fp& __f) : __f_(__f) {}
 
     _LIBCPP_INLINE_VISIBILITY
     void operator()()
     {
         typedef typename __make_tuple_indices<tuple_size<_Fp>::value, 1>::type _Index;
         __execute(_Index());
     }
 
 private:
     template <size_t ..._Indices>
     _LIBCPP_INLINE_VISIBILITY
     void __execute(__tuple_indices<_Indices...>)
     {
-        __invoke(_VSTD::move(_VSTD::get<0>(__f_)), _VSTD::move(_VSTD::get<_Indices>(__f_))...);
+        __invoke(_VSTD::get<0>(_VSTD::move(__f_)), _VSTD::get<_Indices>(_VSTD::move(__f_))...);
     }
 };
 
 #else
 
 template <class _Fp>
 class __call_once_param
 {
-    _Fp __f_;
+    const _Fp& __f_;
 public:
-#ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
-    _LIBCPP_INLINE_VISIBILITY
-    explicit __call_once_param(_Fp&& __f) : __f_(_VSTD::move(__f)) {}
-#else
     _LIBCPP_INLINE_VISIBILITY
     explicit __call_once_param(const _Fp& __f) : __f_(__f) {}
-#endif
 
     _LIBCPP_INLINE_VISIBILITY
     void operator()()
     {
         __f_();
     }
 };
 

 
 template<class _Callable, class... _Args>
 inline _LIBCPP_INLINE_VISIBILITY
 void
 call_once(once_flag& __flag, _Callable&& __func, _Args&&... __args)
 {
     if (__flag.__state_ != ~0ul)
     {
-        typedef tuple<typename decay<_Callable>::type, typename decay<_Args>::type...> _Gp;
-        __call_once_param<_Gp> __p(_Gp(__decay_copy(_VSTD::forward<_Callable>(__func)),
-                                __decay_copy(_VSTD::forward<_Args>(__args))...));
+        typedef tuple<_Callable&&, _Args&&...> _Gp;
+        _Gp __f(_VSTD::forward<_Callable>(__func), _VSTD::forward<_Args>(__args)...);
+        __call_once_param<_Gp> __p(__f);
         __call_once(__flag.__state_, &__p, &__call_once_proxy<_Gp>);
     }
 }
 
 #else  // _LIBCPP_HAS_NO_VARIADICS
 
 template<class _Callable>
 inline _LIBCPP_INLINE_VISIBILITY
 void
-call_once(once_flag& __flag, _Callable __func)
+call_once(once_flag& __flag, const _Callable& __func)
 {
     if (__flag.__state_ != ~0ul)
     {
         __call_once_param<_Callable> __p(__func);
         __call_once(__flag.__state_, &__p, &__call_once_proxy<_Callable>);
     }
 }
 
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_MUTEX

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

 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: libcpp-has-no-threads
 
 // <mutex>
 
 // struct once_flag;
 
 // template<class Callable, class ...Args>
-//   void call_once(once_flag& flag, Callable func, Args&&... args);
+//   void call_once(once_flag& flag, Callable&& func, Args&&... args);
 
 #include <mutex>
 #include <thread>
 #include <cassert>
 
 typedef std::chrono::milliseconds ms;
 
 std::once_flag flg0;
public:
     MoveOnly() {}
     MoveOnly(MoveOnly&&) {}
 
     void operator()(MoveOnly&&)
     {
     }
 };
 
+class NonCopyable
+{
+#if !defined(__clang__)
+   // GCC 4.8 complains about the following being private
+public:
+    NonCopyable(const NonCopyable&)
+    {
+    }
+#else
+    NonCopyable(const NonCopyable&);
+#endif
+public:
+    NonCopyable() {}
+
+    void operator()(int&) {}
+};
+
 #endif
 
 int main()
 {
     // check basic functionality
     {
         std::thread t0(f0);
         std::thread t1(f0);
// check functors with 2 args
         t0.join();
         t1.join();
         assert(init2::called == 5);
     }
     {
         std::once_flag f;
         std::call_once(f, MoveOnly(), MoveOnly());
     }
+    // check LWG2442: call_once() shouldn't DECAY_COPY()
+    {
+        std::once_flag f;
+        int i = 0;
+        std::call_once(f, NonCopyable(), i);
+    }
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 }

www/cxx1z_status.html

<!--   <I>Note: "NAD" means that the issue was deemed "Not a defect"</I> -->
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2427">2427</td><td>Container adaptors as sequence containers, redux</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2428">2428</td><td>"External declaration" used without being defined</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2433">2433</td><td>uninitialized_copy()/etc. should tolerate overloaded operator&</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2434">2434</td><td>shared_ptr::use_count() is efficient</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2437">2437</td><td>iterator_traits::reference can and can't be void</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2438">2438</td><td>std::iterator inheritance shouldn't be mandated</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2439">2439</td><td>unique_copy() sometimes can't fall back to reading its output</td><td>Lenexa</td><td></td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2440">2440</td><td>seed_seq::size() should be noexcept</td><td>Lenexa</td><td>Complete</td></tr>
-  <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2442">2442</td><td>call_once() shouldn't DECAY_COPY()</td><td>Lenexa</td><td></td></tr>
+  <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2442">2442</td><td>call_once() shouldn't DECAY_COPY()</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2448">2448</td><td>Non-normative Container destructor specification</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2454">2454</td><td>Add raw_storage_iterator::base() member</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2455">2455</td><td>Allocator default construction should be allowed to throw</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2458">2458</td><td>N3778 and new library deallocation signatures</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2459">2459</td><td>std::polar should require a non-negative rho</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2464">2464</td><td>try_emplace and insert_or_assign misspecified</td><td>Lenexa</td><td></td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2467">2467</td><td>is_always_equal has slightly inconsistent default</td><td>Lenexa</td><td>Complete</td></tr>
   <tr><td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2470">2470</td><td>Allocator's destroy function should be allowed to fail to instantiate</td><td>Lenexa</td><td></td></tr>