autoconf-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/5] add a separate diversion for shell functions


From: Eric Blake
Subject: Re: [PATCH 1/5] add a separate diversion for shell functions
Date: Sat, 20 Sep 2008 10:45:02 -0600
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16) Gecko/20080708 Thunderbird/2.0.0.16 Mnenhy/0.7.5.666

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Paolo Bonzini on 9/18/2008 8:12 AM:
> This patch is the first in a set to use shell functions for m4sh
> macros AS_BASENAME, AS_DIRNAME, AS_MKDIR_P.  However, most of these
> patches are applicable anyway.

Thanks for your effort on this series!  I agree that we are finally ready
to require shell functions in ./configure scripts.  It will take me some
time to review them all, but let's start at the beginning.

Do you have a repository published with this series available for quick
test?  If nothing else, feel free to use the mob branch on my repo.or.cz
clone:

$ git push address@hidden:/srv/git/autoconf/ericb.git +HEAD:mob

although you'll need to ask me to copy changes off to a more stable
location, since anyone can overwrite the mob branch.

> 
> This patch adds a separate diversion for shell functions, which are
> output after sanitization but before other initialization.  This
> makes sure that _AS_*_PREPARE macros can use the shell functions
> they set.

Nice - as long as we guarantee that shell initialization finds a shell
that supports functions, then this is the right way to ensure functions
are declared first.  However, for languages with more diversions (think
autoconf and autotest), it still makes sense to have as many macros
deferred until after --help/--version is processed, to avoid time lost in
parsing shell functions that are otherwise unused, so we need to remember
to not make the new diversion the catchall location for all functions.

> 
> The argument to AS_REQUIRE is used also later in the series,
> which is why I have not inlined it in AS_REQUIRE_SHELL_FN.
> 
> 2008-09-18  Paolo Bonzini  <address@hidden>
> 
>       * lib/m4sugar/m4sh.m4 (M4SH-INIT-FN): New diversion.
>       (AS_REQUIRE): Accept diversion parameter.
>       (AS_REQUIRE_SHELL_FN): Use it.

Looks okay, except for these nits:

> 
> @@ -100,10 +103,11 @@ m4_copy([_m4_divert(M4SH-INIT)], [_m4_divert(NOTICE)])
>  ## ------------------------- ##
>  
>  
> -# AS_REQUIRE(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK])
> -# -----------------------------------------------------------
> +# AS_REQUIRE(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK],
> +#            [DIVERSION = M4SH-INIT])
> +# ------------------------------------------------------------------------

trim back those ---- to match the length of the longest line.

>  # BODY-TO-EXPAND is some initialization which must be expanded in the
> -# M4SH-INIT section when expanded (required or not).  This is very
> +# given direction section when expanded (required or not).  This is very

s/direction section/diversion/

>  # different from m4_require.  For instance:
>  #
>  #      m4_defun([_FOO_PREPARE], [foo=foo])
> @@ -140,7 +144,8 @@ m4_copy([_m4_divert(M4SH-INIT)], [_m4_divert(NOTICE)])
>  #
>  m4_define([AS_REQUIRE],
>  [m4_provide_if([$1], [],
> -            [m4_divert_text([M4SH-INIT], [m4_default([$2], [$1])])])])
> +            [m4_divert_text(m4_default([$3], [M4SH-INIT]),
> +                            [m4_default([$2], [$1])])])])

This is a subtle semantic change if M4SH or INIT is defined as a macro
name, since m4_default expands its argument (although we are unlikely to
be using macro names that conflict like that, it never hurts to play it
safe).  Maybe it is time to introduce an m4sugar macro m4_default_quoted,
which does NOT expand its arguments - I have several places in mind that
could use it, in addition to this part of your patch.  What do you think,
should I tackle that before we apply this series?

# m4_default_quoted([TEXT], DEFAULT)
# ----------------------------------
# If TEXT is not the empty string, output it without expansion;
# otherwise output DEFAULT without expansion.
# Contrast this with m4_default, which expands the final result:
#   m4_define([active], [ACTIVE])
#   m4_default([active], [default]) => ACTIVE
#   m4_default([], [active]) => ACTIVE
#   m4_default_quoted([active], [default]) => active
#   m4_default_quoted([], [active]) => active
m4_define([m4_default_quoted],
[m4_if([$1], [], [[$2]], [[$1]])])

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkjVKI4ACgkQ84KuGfSFAYDItQCfdcoNiyoG4XQVdcVvUfnrX1x7
c18An0qa4xOa5SB/BHNB3kelgVyZtjv4
=+xjG
-----END PGP SIGNATURE-----




reply via email to

[Prev in Thread] Current Thread [Next in Thread]