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

[Eric Sunshine <sunshine@sunshineco.com>]

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