Re: [PATCH] Remove __PTHREAD_MUTEX_HAVE_ELISION undefined warning

[Roland McGrath <roland at hack dot frob dot com>]

> 	* nptl/sysdeps/pthread/bits/pthread-elision.h: New header: define
> 	default lock elision support and defines.
> 	* nptl/sysdeps/pthread/pthread.h: Include pthread-elision.h.

Say bits/pthread-elision.h, or since it follows the addition, just "it".

> 	* nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
> 	(__PTHREAD_MUTEX_HAVE_ELISION): Undefine.

Say "Macro removed."  Saying "Undefine" implies #undef.

> 	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h: New
> 	header: x86 specific lock elision support.
> 	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h:

No colon at the end of the line when there are (...) sections in the entry.

> 	(__PTHREAD_MUTEX_HAVE_ELISION): Definition moved to specific lock
> 	elision header.

That's fine, but canonical style is:

	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
	(__PTHREAD_MUTEX_HAVE_ELISION): Macro moved to ...
	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h:
	... this new file.

> 	* nptl/Makefile: Add bits/pthread-elision.h to install.

Should be:

	* nptl/Makefile (headers): Add bits/pthread-elision.h.

But I'd put this right after the pthread.h log line, and just say, "Add it."

> +#ifndef _PTHREAD_H
> +# error "Never include this file directly.  Use <pthread.h> instead"
> +#endif

Needs a trailing period.

> +/* Define it if the architecture supports lock elision using transactional
> +   memory or a similar facility.  It changes the mutex initializers to add
> +   the required elision fields.  Currently, three value are possible:
> +   * 0: No elision support, default value.
> +   * 1: Elision support for 64 bits.
> +   * 2: Elision support for 32 bits.  */
> +#define __PTHREAD_MUTEX_HAVE_ELISION 0

s/value/values/

I don't understand what "support for 64 bits" or "support for 32 bits"
means.  OK, I've looked at bits/pthreadtypes.h so I do understand.  But it
seems pretty wrong to pretend this is a generic 32/64 sort of thing when it
is really just about the x86-private layout of pthread_mutex_t.  It seems
more proper to have bits/pthreadtypes.h just define __PTHREAD_SPINS.  

That can be a separate cleanup if you want, and others may want to kibitz.
But that might involve dropping the header you're adding here, so maybe you
want to just resolve it now.

If you want to go ahead with this change, then it's OK with the other nits
above and this comment rewritten to describe concretely what the macro
means.  In actual usage so far, it doesn't actually mean anything about
elision support per se.  It just means something about how the fields of
pthread_mutex_t are structured and hence what the initializer must look like.
If that's all it's for, it should be made clear.


Thanks,
Roland