[cfe-commits] [PATCH] mutex: unify __mutex_base constructors

[Richard Smith]

On Mon, Dec 10, 2012 at 8:16 AM, Saleem Abdulrasool
<compnerd at compnerd.org> wrote:
> On Monday, December 10, 2012, Howard Hinnant wrote:
>>
>> On Dec 10, 2012, at 2:50 AM, Saleem Abdulrasool <compnerd at compnerd.org>
>> wrote:
>>
>> > Use the _LIBCPP_CONSTEXPR macro to allow the preprocessor to determine
>> > if
>> > constexpr should be applied to the constructor.  This is required to
>> > unify the
>> > constructor definition.
>> >
>> > Unify the constructor declarations, moving the member variable
>> > initialisation
>> > into the initialiser-list.  This requires an ugly C-style cast to ensure
>> > portability across all implementations.
>> > PTHREAD_{MUTEX,COND}_INITIALIZER may be
>> > an aggregate initialiser, which requires an explicit type conversion for
>> > the
>> > compound literal.
>> >
>> > http://llvm-reviews.chandlerc.com/D194
>> >
>> > Files:
>> >  include/__mutex_base
>> >
>> > Index: include/__mutex_base
>> > ===================================================================
>> > --- include/__mutex_base
>> > +++ include/__mutex_base
>> > @@ -37,13 +37,9 @@
>> >     pthread_mutex_t __m_;
>> >
>> > public:
>> > -    _LIBCPP_INLINE_VISIBILITY
>> > -#ifndef _LIBCPP_HAS_NO_CONSTEXPR
>> > -     constexpr mutex() _NOEXCEPT : __m_(PTHREAD_MUTEX_INITIALIZER) {}
>> > -#else
>> > -     mutex() _NOEXCEPT {__m_ =
>> > (pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER;}
>> > -#endif
>> > -     ~mutex();
>> > +    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR mutex() _NOEXCEPT
>> > +        : __m_((pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER) {}
>> > +    ~mutex();
>> >
>> > private:
>> >     mutex(const mutex&);// = delete;
>> > @@ -303,12 +299,8 @@
>> > {
>> >     pthread_cond_t __cv_;
>> > public:
>> > -    _LIBCPP_INLINE_VISIBILITY
>> > -#ifndef _LIBCPP_HAS_NO_CONSTEXPR
>> > -    constexpr condition_variable() : __cv_(PTHREAD_COND_INITIALIZER) {}
>> > -#else
>> > -    condition_variable() {__cv_ =
>> > (pthread_cond_t)PTHREAD_COND_INITIALIZER;}
>> > -#endif
>> > +    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR condition_variable()
>> > +        : __cv_((pthread_cond_t)PTHREAD_COND_INITIALIZER) {}
>> >     ~condition_variable();
>> >
>> > private:
>> > <D194.1.patch>_______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>> Is this change just for stylistic reasons, or is it required to get
>> something working?
>
>
>  It is simply a stylistic/cleanup change.  There should be no change in
> functionality.

This code is fraught with portability peril, so just changing it for
stylistic reasons is not a good idea. When PTHREAD_MUTEX_INITIALIZER
is a braced-init-list...

-     constexpr mutex() _NOEXCEPT : __m_(PTHREAD_MUTEX_INITIALIZER) {}

For a C++11 implementation, this is the most compatible
implementation. It relies upon a GNU extension in the case that
pthread_mutex_t is an array type, and is otherwise valid C++11. It is
not valid in C++98.

-     mutex() _NOEXCEPT {__m_ = (pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER;}

The fallback codepath (for the C++98 case) uses a C99
compound-initializer, and doesn't work *at all* for array types.

+        : __m_((pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER) {}

Your replacement has the same problems as the fallback code. That is,
it breaks if pthread_mutex_t is an array type, and uses a C99
compound-initializer.