Re: use of AC_SUBST

[Jim Meyering]

Bruno Haible wrote:
>> > Likewise with LIB_GETHRXTIME. How about this proposed fix?
>>
>> Good catch.
>
> Patch applied.
>
>> However, please make these two lines adjacent:
>> (pulling the AC_SUBST "up")
>> ...
>> I nearly moved the AC_SUBST lines in the changes I made recently,
>> but opted not to: I didn't want to mix bug-fix and clean-up fixes.
>
> You didn't want to mix bug fix and cleanup changes on Thursday, so you cannot
> expect me to do this today :-)

I would never propose something that seems hypocritical to me.
You should know me better than to impugn, even with a smiley.

This is different, since your patch was already moving the AC_SUBST use,
which was technically unnecessarily.

The situations are really very similar.
I opted not to move it at all, rather than to impose my style.
If you want to be pedantic, you would not have moved it at all,
rather than impose *your* style.  However, since in your case,
the assignment had to move from one function to another, you
have a slightly better case for moving the AC_SUBST use.

Actually, what I would have liked was an autoconf macro with the
semantics of AC_REQUIRE, but that could be applied to arbitrary text,
not just functions.  Then we could have left both uses of LIB_GETHRXTIME
in the callee, and told *autoconf* to hoist the initialization code via e.g.,

    AC_REQUIRE_FOO([LIB_GETHRXTIME=])


>> Since AC_SUBST can be used anywhere, it's slightly more
>> maintainer-friendly to keep these two references to LIB_GETHRXTIME close.
>>     LIB_GETHRXTIME=
>>     AC_SUBST([LIB_GETHRXTIME])
>
> I disagree. The traditional way to use AC_SUBST is to initialize the variable,

"traditional" does not always equate to "best practice".
IMHO, last use is last use, and we shouldn't perpetrate the
myth that AC_SUBST must follow the final assignment.

> then compute its value, and finally do AC_SUBST of the variable. This coding
> style has the advantage that, as a maintainer, when you search for assignments
> to the variable, the possible locations of such assignments are limited. In
> other words, the initialization and the AC_SUBST act as an opening and closing
> brace, respectively.

I've just made this change:

>From 6cb63a9700e8b21e1ad765b0a8e6cfccc9c162ea Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 18 Oct 2009 18:53:35 +0200
Subject: [PATCH] m4: stylistic-only: hoist AC_SUBST to be adjacent to 
initialization

Declare a variable like LIB_CLOCK_GETTIME to be AC_SUBSTituted
right after its initialization, rather than farther down.
Keeping these in close proximity makes it easier to ensure
that each such variable is initialized.  E.g.,

    LIB_CLOCK_GETTIME=
    AC_SUBST([LIB_CLOCK_GETTIME])

This change also increments these serial numbers.
* m4/clock_time.m4 (gl_CLOCK_TIME): Hoist AC_SUBST use.
* m4/euidaccess.m4 (gl_PREREQ_EUIDACCESS): Likewise.
* m4/nanosleep.m4 (gl_FUNC_NANOSLEEP): Likewise.
---
 ChangeLog        |   16 ++++++++++++++++
 m4/clock_time.m4 |    4 ++--
 m4/euidaccess.m4 |    4 ++--
 m4/nanosleep.m4  |    4 ++--
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d176b0a..b583da3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2009-10-18  Jim Meyering  <address@hidden>
+
+       m4: stylistic-only: hoist AC_SUBST to be adjacent to initialization
+       Declare a variable like LIB_CLOCK_GETTIME to be AC_SUBSTituted
+       right after its initialization, rather than farther down.
+       Keeping these in close proximity makes it easier to ensure
+       that each such variable is initialized.  E.g.,
+
+           LIB_CLOCK_GETTIME=
+           AC_SUBST([LIB_CLOCK_GETTIME])
+
+       This change also increments these serial numbers.
+       * m4/clock_time.m4 (gl_CLOCK_TIME): Hoist AC_SUBST use.
+       * m4/euidaccess.m4 (gl_PREREQ_EUIDACCESS): Likewise.
+       * m4/nanosleep.m4 (gl_FUNC_NANOSLEEP): Likewise.
+
 2009-10-18  Bruno Haible  <address@hidden>

        Don't let environment variables perturb build.
diff --git a/m4/clock_time.m4 b/m4/clock_time.m4
index d67f3df..d11e106 100644
--- a/m4/clock_time.m4
+++ b/m4/clock_time.m4
@@ -1,4 +1,4 @@
-# clock_time.m4 serial 9
+# clock_time.m4 serial 10
 dnl Copyright (C) 2002-2006, 2009 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -21,11 +21,11 @@ AC_DEFUN([gl_CLOCK_TIME],
   # programs in the package would end up linked with that potentially-shared
   # library, inducing unnecessary run-time overhead.
   LIB_CLOCK_GETTIME=
+  AC_SUBST([LIB_CLOCK_GETTIME])
   gl_saved_libs=$LIBS
     AC_SEARCH_LIBS([clock_gettime], [rt posix4],
                    [test "$ac_cv_search_clock_gettime" = "none required" ||
                     LIB_CLOCK_GETTIME=$ac_cv_search_clock_gettime])
-    AC_SUBST([LIB_CLOCK_GETTIME])
     AC_CHECK_FUNCS([clock_gettime clock_settime])
   LIBS=$gl_saved_libs
 ])
diff --git a/m4/euidaccess.m4 b/m4/euidaccess.m4
index 5e8adac..1f748f4 100644
--- a/m4/euidaccess.m4
+++ b/m4/euidaccess.m4
@@ -1,4 +1,4 @@
-# euidaccess.m4 serial 11
+# euidaccess.m4 serial 12
 dnl Copyright (C) 2002-2009 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -43,11 +43,11 @@ AC_DEFUN([gl_PREREQ_EUIDACCESS], [
   # programs in the package would end up linked with that potentially-shared
   # library, inducing unnecessary run-time overhead.
   LIB_EACCESS=
+  AC_SUBST([LIB_EACCESS])
   gl_saved_libs=$LIBS
     AC_SEARCH_LIBS([eaccess], [gen],
                    [test "$ac_cv_search_eaccess" = "none required" ||
                     LIB_EACCESS=$ac_cv_search_eaccess])
-    AC_SUBST([LIB_EACCESS])
     AC_CHECK_FUNCS([eaccess])
   LIBS=$gl_saved_libs
 ])
diff --git a/m4/nanosleep.m4 b/m4/nanosleep.m4
index 663fedf..d991b61 100644
--- a/m4/nanosleep.m4
+++ b/m4/nanosleep.m4
@@ -1,4 +1,4 @@
-# serial 27
+# serial 28

 dnl From Jim Meyering.
 dnl Check for the nanosleep function.
@@ -25,6 +25,7 @@ AC_DEFUN([gl_FUNC_NANOSLEEP],
  # Solaris 2.5.1 needs -lposix4 to get the nanosleep function.
  # Solaris 7 prefers the library name -lrt to the obsolescent name -lposix4.
  LIB_NANOSLEEP=
+ AC_SUBST([LIB_NANOSLEEP])
  AC_SEARCH_LIBS([nanosleep], [rt posix4],
                 [test "$ac_cv_search_nanosleep" = "none required" ||
                  LIB_NANOSLEEP=$ac_cv_search_nanosleep])
@@ -113,7 +114,6 @@ AC_DEFUN([gl_FUNC_NANOSLEEP],
     gl_PREREQ_NANOSLEEP
   fi

- AC_SUBST([LIB_NANOSLEEP])
  LIBS=$nanosleep_save_libs
 ])

--
1.6.5.1.258.g5b20