On 09.07.2012, at 16:50, Junio C Hamano wrote:

> Max Horn <m...@quendi.de> writes:
>> The configure script checks whether certain flags / libraries are
>> required to use pthreads. But so far it did not consider the possibility
>> that no extra compiler flags are needed (as is the case on Mac OS X). As
>> a result, configure would always add "-mt" to the list of flags. This in
>> turn triggered a warning in clang about an unknown argument.
>> To solve this, we now first check if pthreads work without extra flags.
>> Signed-off-by: Max Horn <m...@quendi.de>
>> ---
>> configure.ac | 2 +-
>> 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
>> diff --git a/configure.ac b/configure.ac
>> index 4e9012f..d767ef3 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
>> # -D_REENTRANT' or some such.
>> elif test -z "$PTHREAD_CFLAGS"; then
>>   threads_found=no
>> -  for opt in -mt -pthread -lpthread; do
>> +  for opt in "" -mt -pthread -lpthread; do
> Hmph.  Would it work to append the new empty string at the end of
> the existing list, as opposed to prepending it?

No, because that loop aborts on the first match that "works". Since no flags 
are necessary on OS X, but adding "-mt" to the flags "works" in the sense that 
it does nothing (except triggering a warning about an unknown argument), we 
need to check the empty string before "-mt" that. 

>  I'd prefer a
> solution that is order independent, or if the change is order
> dependent, then a comment to warn others from changing it later.

Well, such checks are normally always order dependant, too (looking at many 
other autoconf tests out there). OK, so we could first test all four 
possibilities, recording for each whether it works or not. But after doing 
that, we still need to establish an order, in case more than one "works" 
according to the linker test. Simply using the order is the easiest way for 
that and works well in practice. And it avoid unnecessary, time consuming 
checks ;).

Regardless of all that, of course I can add a comment emphasising that the 
order here is important. (Although IMHO that should be self-evident for an 
autoconf test.)

>>      old_CFLAGS="$CFLAGS"
>>      CFLAGS="$opt $CFLAGS"
>>      AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])
> Perhaps "for linking with POSIX Threads" would make it clearer, as
> CFLAGS (rather, PTHREAD_CFLAGS) has been checked earlier separately.

Well, talking about clarity, looking at line 188 it says
   AC_MSG_NOTICE([Will try -pthread then -lpthread to enable POSIX Threads])

which is also bad: It is out-of-date (even before my patch, as it doesn't 
mention -mt), will easily get out of sync with reality again, and in any case 
contains information that normally shouldn't be printed anyway. 

Beyond that, the pthread code checks only for -pthread and -lpthread, thus 
leaving many systems out. Indeed, it might be worth a thought dropping the 
custom pthread detection code in git's configure.ac and instead using 
AX_PTHREAD <http://www.gnu.org/software/autoconf-archive/ax_pthread.html>, 
which covers many more systems out of the box. 

But for now, I really would just want to make this simple trivial fix that 
avoids tons of pointless warnings when compiling git on Mac OS X 10.7 ... ;).


To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to