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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 7, 2015, 21:37 UTC
Message-ID: <CAPig+cSx9FZtn=RH25OX97EBmbnKfVT66WsgtqenVZpm8LBjHQ@mail.gmail.com>
In-Reply-To: <1420662236-27593-3-git-send-email-reubenhwk@gmail.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]).

Show 12 quoted lines

> ---
> 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.

Show 16 quoted lines

>  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]))

Show 6 quoted lines

> +
> +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).

Show 9 quoted lines

> +#
> +# 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