Re: [PATCH 1/5] Clean pthread types namespace for all archs.

[Juan Manuel Torres Palma <j dot m dot torrespalma at gmail dot com>]

> It would be more useful if the default header had comments explaining the
> interface - that is, what macros this header should define and what the
> semantics of their definitions are.
>

/* This file is required to have a shared types definition
 * between POSIX threads and C11 threads. It allows to
 * header files like pthread.h and thread.h to define their types
 * with the same size and fields without corrupting namespaces.
 *
 * The interface is as follows:
 *
 * - $ARCH/bits/thread-shared-types.h. Defines specific arch
 *   structures for mutex and condvar. There is a single definition
 *   for each architecture. The macros defined are
 *   __PTHREAD_MUTEX_T_CONTENT and __PTHREAD_COND_T_CONTENT
 *
 * - bits/pthreadtypes-common.h. Works as a generic file that defines
 *   common pthread types like pthread_mutex_t and pthread_cond_t
 *   based on the arch specific definition included from
 *   thread-shared-types.h.
 *
 * - $ARCH/pthreadtypes.h. Include pthreadtypes-common.h to have access
 *   to pthread types and expose them for the other types and functions
 *   that require it.
 */

Is anything like this suitable?

> __PTHREAD_MUTEX_HAVE_PREV is describing the presence of absence of a field
> in (a substructure of) pthread_mutex_t.  I think it would be better for
> the definitions of this macro to go in thread-shared-types.h, so that it
> goes close to where the structure contents in question are defined and
> it's easier to see whether the definition or its absence is correct.  As
> is, the patch leaves it in pthreadtypes.h, having moved the containing
> structure definition.

Thanks for pointing out, just noticed there is an inconsistency,
leaving __PTHREAD_MUTEX_HAVE_PREV in pthreadtypes.h for some
architectures, while in some others moving it to
thread-shared-types.h.

Cheers.

2015-08-14 8:40 GMT+09:00 Joseph Myers <joseph@codesourcery.com>:
> On Mon, 10 Aug 2015, Juan Manuel Torres Palma wrote:
>
>> diff --git a/bits/thread-shared-types.h b/bits/thread-shared-types.h
>> new file mode 100644
>> index 0000000..0e26952
>> --- /dev/null
>> +++ b/bits/thread-shared-types.h
>> @@ -0,0 +1 @@
>> +/* No thread support.  */
>
> It would be more useful if the default header had comments explaining the
> interface - that is, what macros this header should define and what the
> semantics of their definitions are.
>
>>  /* Data structures for mutex handling.  The structure of the attribute
>>     type is not exposed on purpose.  */
>> -typedef union
>> -{
>> -  struct __pthread_mutex_s
>> -  {
>> -    int __lock;
>> -    unsigned int __count;
>> -    int __owner;
>> -    unsigned int __nusers;
>> -    int __kind;
>> -    int __spins;
>> -    __pthread_list_t __list;
>> -#define __PTHREAD_MUTEX_HAVE_PREV    1
>> -  } __data;
>> -  char __size[__SIZEOF_PTHREAD_MUTEX_T];
>> -  long int __align;
>> -} pthread_mutex_t;
>
> __PTHREAD_MUTEX_HAVE_PREV is describing the presence of absence of a field
> in (a substructure of) pthread_mutex_t.  I think it would be better for
> the definitions of this macro to go in thread-shared-types.h, so that it
> goes close to where the structure contents in question are defined and
> it's easier to see whether the definition or its absence is correct.  As
> is, the patch leaves it in pthreadtypes.h, having moved the containing
> structure definition.
>
> Arguably the same applies to __PTHREAD_SPINS: it most clearly goes
> alongside the structure definition.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com



-- 
Juan Manuel Torres Palma.
Computer Science Student at Universidad de Granada.