[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: use of AC_SUBST
From: |
Jim Meyering |
Subject: |
Re: use of AC_SUBST |
Date: |
Sun, 18 Oct 2009 19:00:55 +0200 |
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
Re: [PATCH] don't let environment settings perturb build, Ralf Wildenhues, 2009/10/31