On Wed, Jan 7, 2015 at 1:37 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@xxxxxxxxx> 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])]) I *think* there's an issue with this simplification as used right here. In the 'no' case, HAVE_CLOCK_MONOTONIC *must* be undefined by setting it equal to nothing HAVE_CLOCK_MONOTONIC= So that the setting in config.mak.uname 'HAVE_CLOCK_MONOTINIC = YesPlease' will be overridden. So this one needs to stay as is. > >> +# >> # 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html