Re: [PATCH 2/3] configure.ac: check for clock_gettime and CLOCK_MONOTONIC

> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
>> systems being used in production.  This change makes compiling git
>> less tedious on older platforms without CLOCK_MONOTONIC.
>
> The second sentence is implied by the very presence of this patch,
> thus adds no value. I, personally, would drop it.
>
> Also, your sign-off is missing (as mentioned in my previous review[1]).
>
>> ---
>> diff --git a/Makefile b/Makefile
>> index 7482a4d..af551a0 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1382,6 +1382,10 @@ ifdef HAVE_CLOCK_GETTIME
>>         EXTLIBS += -lrt
>>  endif
>>
>> +ifdef HAVE_CLOCK_MONOTONIC
>> +       BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
>> +endif
>
> You need to document this new Makefile variable (HAVE_CLOCK_MONOTONIC)
> at the top of Makefile (as mentioned in my previous review[1]), so
> that people who build without running 'configure' will know that they
> may need to tweak it.
>
>>  ifeq ($(TCLTK_PATH),)
>>  NO_TCLTK = NoThanks
>>  endif
>> diff --git a/configure.ac b/configure.ac
>> index dcc4bf0..424dec5 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -923,6 +923,28 @@ AC_CHECK_LIB([iconv], [locale_charset],
>>                       [CHARSET_LIB=-lcharset])])
>>  GIT_CONF_SUBST([CHARSET_LIB])
>>  #
>> +# Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
>> +GIT_CHECK_FUNC(clock_gettime,
>> +[HAVE_CLOCK_GETTIME=YesPlease],
>> +[HAVE_CLOCK_GETTIME=])
>> +GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
>
> You could simplify the above four lines to this one-liner:
>
>     GIT_CHECK_FUNC(clock_gettime,
>         GIT_CONF_SUBST([HAVE_CLOCK_GETTIME], [YesPlease]))
>
>> +
>> +AC_DEFUN([CLOCK_MONOTONIC_SRC], [
>> +AC_LANG_PROGRAM([[
>> +#include <time.h>
>> +clockid_t id = CLOCK_MONOTONIC;
>> +]], [])])
>
> No need to pass empty trailing arguments in m4. It's customary to drop
> them altogether (since they are implied).
>
>> +#
>> +# Define HAVE_CLOCK_MONOTONIC=YesPlease if CLOCK_MONOTONIC is available.
>> +AC_MSG_CHECKING([for CLOCK_MONOTONIC])
>> +AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
>> +       [AC_MSG_RESULT([yes])
>> +       HAVE_CLOCK_MONOTONIC=YesPlease],
>> +       [AC_MSG_RESULT([no])
>> +       HAVE_CLOCK_MONOTONIC=])
>> +GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])
>
> Ditto regarding simplification:
>
>     AC_MSG_CHECKING([for CLOCK_MONOTONIC])
>     AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
>         [AC_MSG_RESULT([yes])
>         GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC], [YesPlease])],
>         [AC_MSG_RESULT([no])])

>
>> +#
>>  # Define NO_SETITIMER if you don't have setitimer.
>>  GIT_CHECK_FUNC(setitimer,
>>  [NO_SETITIMER=],
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261630