Re: [PATCH v2 1/4] Replace 'test "${var+set}" = set' with '[ ${var+y} ]'

[Eric Blake]

On 04/09/2015 06:43 AM, Bernhard Reutner-Fischer wrote:
> The latter is faster to parse (let's say) and is already used in other
> spots.

The comment is no longer accurate (we aren't using [ ${var+y} ] in other
places, prior to this patch).

> 
> Tested with e.g. dash and busybox ash.
> No regressions.

dash and busybox are not all the shells that need testing, but I am
fairly confident that this is safe.  Still, I think the use of [] is
dangerous, and would prefer that we stick with the 'test' form, if only
because with the [] form, we HAVE to audit that none of the uses occur
within a macro that is likely to be re-expanded and lose the [] to m4.

> 
> Please install.

This message doesn't need to be in the git log; rather, if you feel the
need to make comments relevant only to the mailing list...

> 
> 2015-04-08  Bernhard Reutner-Fischer  <address@hidden>
> 
>       * lib/autoconf/general.m4 (_AC_ENABLE_IF_ACTION): Use
>       '[ ${var+set} ]' instead of 'test "${var+set}" = set.
>       (AC_CACHE_SAVE): Likewise.
>       * lib/autoconf/lang.m4 (ac_link_default): Likewise.
>       * lib/autoconf/programs.m4 (AC_PROG_INSTALL, AC_PROG_MKDIR_P): Likewise.
>       * lib/autoconf/status.m4 (_AC_OUTPUT_MAIN_LOOP): Likewise.
>       * lib/autotest/general.m4 (at_fn_create_debugging_script, Driver
>       loop.): Likewise.
>       * lib/m4sugar/m4sh.m4 (_AS_DETECT_BETTER_SHELL,
>       _AS_SHELL_SANITIZE, _AS_PATH_SEPARATOR_PREPARE): Likewise.
>       * tests/base.at (AC_CACHE_CHECK): Likewise.
>       * tests/m4sh.at (LINENO stack): Use test "".
> 
> Signed-off-by: Bernhard Reutner-Fischer <address@hidden>
> ---

... sticking them here is the best place.

>  lib/autoconf/general.m4  |    4 ++--
>  lib/autoconf/lang.m4     |    2 +-
>  lib/autoconf/programs.m4 |    4 ++--
>  lib/autoconf/status.m4   |    8 ++++----
>  lib/autotest/general.m4  |    4 ++--
>  lib/m4sugar/m4sh.m4      |    8 ++++----
>  tests/base.at            |    2 +-
>  tests/m4sh.at            |    2 +-
>  8 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/autoconf/general.m4 b/lib/autoconf/general.m4
> index 2d1a291..adf4f39 100644
> --- a/lib/autoconf/general.m4
> +++ b/lib/autoconf/general.m4
> @@ -1467,7 +1467,7 @@ _AC_ENABLE_IF_ACTION([$1], m4_translit([$2], [-+.], 
> [___]), [$3], [$4])
>  m4_define([_AC_ENABLE_IF_ACTION],
>  [m4_append_uniq([_AC_USER_OPTS], [$1_$2], [
>  ])dnl
> -AS_IF([test "${$1_$2+set}" = set], [$1val=$$1_$2; $3], [$4])dnl
> +AS_IF([[[ ${$1_$2+y} ]]], [$1val=$$1_$2; $3], [$4])dnl

This code snippet definitely appears multiple times in typical configure
files, so it's nice to shave in size.

However, this is one of those cases where you may have broken things.
If $1 or $2 is an m4 macro name, the old code would recursively expand
that macro, the new code will not.  (I don't know if someone ever passed
$1 or $2 as a macro name, but my point is that avoiding the need to
audit whether subtle semantic changes matter is the better approach).
That, and '[[[' looks awkward.

>  ])
>  
>  # AC_ARG_ENABLE(FEATURE, HELP-STRING, [ACTION-IF-TRUE], [ACTION-IF-FALSE])
> @@ -2044,7 +2044,7 @@ _AC_CACHE_DUMP() |
>       /^ac_cv_env_/b end
>       t clear
>       :clear
> -     s/^\([^=]*\)=\(.*[{}].*\)$/test "${\1+set}" = set || &/
> +     s/^\([^=]*\)=\(.*[{}].*\)$/[ ${\1+y} ] || &/

Given the [] earlier in the line, it looks like the added use of [] here
is at the right level of nesting, so this one is probably safe. But this
macro is not frequently output into configure scripts, so it is not
saving as much file size.

>       t end
>       s/^\([^=]*\)=\(.*\)$/\1=${\1=\2}/
>       :end'] >>confcache
> diff --git a/lib/autoconf/lang.m4 b/lib/autoconf/lang.m4
> index 2e30f50..bee633f 100644
> --- a/lib/autoconf/lang.m4
> +++ b/lib/autoconf/lang.m4
> @@ -553,7 +553,7 @@ do
>       # certainly right.
>       break;;
>      *.* )
> -     if test "${ac_cv_exeext+set}" = set && test "$ac_cv_exeext" != no;
> +     if [[ ${ac_cv_exeext+y} ]] && test "$ac_cv_exeext" != no;

This one's fine semantic wise, but looks dumb consistency-wise to mix []
and test in the same if statement.  We prefer test for a reason, so we
might as well always use it.

>       then :; else
>          ac_cv_exeext=`expr "$ac_file" : ['[^.]*\(\..*\)']`
>       fi
> diff --git a/lib/autoconf/programs.m4 b/lib/autoconf/programs.m4
> index 59df1a2..931177b 100644
> --- a/lib/autoconf/programs.m4
> +++ b/lib/autoconf/programs.m4
> @@ -586,7 +586,7 @@ esac
>  ])
>  rm -rf conftest.one conftest.two conftest.dir
>  ])dnl
> -  if test "${ac_cv_path_install+set}" = set; then
> +  if [[ ${ac_cv_path_install+y} ]]; then

This one seems fine in isolation.

>      INSTALL=$ac_cv_path_install
>    else
>      # As a last resort, use the slow shell script.  Don't cache a
> @@ -680,7 +680,7 @@ if test -z "$MKDIR_P"; then
>        done
>         done])])
>    test -d ./--version && rmdir ./--version
> -  if test "${ac_cv_path_mkdir+set}" = set; then
> +  if [[ ${ac_cv_path_mkdir+y} ]]; then

as does this one

>      MKDIR_P="$ac_cv_path_mkdir -p"
>    else
>      # As a last resort, use the slow shell script.  Don't cache a
> diff --git a/lib/autoconf/status.m4 b/lib/autoconf/status.m4
> index ef9d521..7ccc847 100644
> --- a/lib/autoconf/status.m4
> +++ b/lib/autoconf/status.m4
> @@ -1604,16 +1604,16 @@ AC_DEFUN([_AC_OUTPUT_MAIN_LOOP],
>  # bizarre bug on SunOS 4.1.3.
>  if $ac_need_defaults; then
>  m4_ifdef([_AC_SEEN_CONFIG(FILES)],
> -[  test "${CONFIG_FILES+set}" = set || CONFIG_FILES=$config_files
> +[  [[ ${CONFIG_FILES+y} ]] || CONFIG_FILES=$config_files
>  ])dnl
>  m4_ifdef([_AC_SEEN_CONFIG(HEADERS)],
> -[  test "${CONFIG_HEADERS+set}" = set || CONFIG_HEADERS=$config_headers
> +[  [[ ${CONFIG_HEADERS+y} ]] || CONFIG_HEADERS=$config_headers
>  ])dnl
>  m4_ifdef([_AC_SEEN_CONFIG(LINKS)],
> -[  test "${CONFIG_LINKS+set}" = set || CONFIG_LINKS=$config_links
> +[  [[ ${CONFIG_LINKS+y} ]] || CONFIG_LINKS=$config_links
>  ])dnl
>  m4_ifdef([_AC_SEEN_CONFIG(COMMANDS)],
> -[  test "${CONFIG_COMMANDS+set}" = set || CONFIG_COMMANDS=$config_commands
> +[  [[ ${CONFIG_COMMANDS+y} ]] || CONFIG_COMMANDS=$config_commands

This pattern for setting a variable to a default value also occurs in
the manual under the section on ${var=value}; we should probably update
that recommendation to the shorter form, since we should follow our own
documentation.  Or we should determine if we can portably use the even
shorter ${CONFIG_COMMANDS=$config_commands} these days, after properly
rejecting ancient broken shells.

>  ])dnl
>  fi
>  
> diff --git a/lib/autotest/general.m4 b/lib/autotest/general.m4
> index e70e326..59242ab 100644
> --- a/lib/autotest/general.m4
> +++ b/lib/autotest/general.m4
> @@ -363,7 +363,7 @@ at_fn_create_debugging_script ()
>  {
>    {
>      echo "#! /bin/sh" &&
> -    echo 'test "${ZSH_VERSION+set}" = set dnl
> +    echo '[[ ${ZSH_VERSION+y} ]] dnl
>  && alias -g '\''${1+"address@hidden"}'\''='\''"address@hidden"'\''' &&
>      AS_ECHO(["cd '$at_dir'"]) &&
>      AS_ECHO(["exec \${CONFIG_SHELL-$SHELL} \"$at_myself\" -v -d ]dnl
> @@ -1367,7 +1367,7 @@ dnl Unfortunately, ksh93 fork-bombs when we send TSTP, 
> so send STOP
>  dnl if this might be ksh (STOP prevents possible TSTP handlers inside
>  dnl AT_CHECKs from running).  Then stop ourselves.
>         at_sig=TSTP
> -       test "${TMOUT+set}" = set && at_sig=STOP
> +       [[ ${TMOUT+y} ]] && at_sig=STOP
>         kill -$at_sig $at_pids 2>/dev/null
>       fi
>       kill -STOP $$
> diff --git a/lib/m4sugar/m4sh.m4 b/lib/m4sugar/m4sh.m4
> index 603466f..1bdf21c 100644
> --- a/lib/m4sugar/m4sh.m4
> +++ b/lib/m4sugar/m4sh.m4
> @@ -100,7 +100,7 @@ _$0
>  # This is the part of AS_BOURNE_COMPATIBLE which has to be repeated inside
>  # each instance.
>  m4_define([_AS_BOURNE_COMPATIBLE],
> -[AS_IF([test -n "${ZSH_VERSION+set}" && (emulate sh) >/dev/null 2>&1],
> +[AS_IF([test -n "${ZSH_VERSION+y}" && (emulate sh) >/dev/null 2>&1],
>   [emulate sh
>    NULLCMD=:
>    [#] Pre-4.2 versions of Zsh do word splitting on ${1+"address@hidden"}, 
> which
> @@ -252,7 +252,7 @@ dnl Unfortunately, $as_me isn't available here.
>      AS_IF([test x$as_have_required = xno],
>        [AS_ECHO(["$[]0: This script requires a shell more modern than all"])
>    AS_ECHO(["$[]0: the shells that I found on your system."])
> -  if test x${ZSH_VERSION+set} = xset ; then
> +  if [[ ${ZSH_VERSION+y} ]]; then
>      AS_ECHO(["$[]0: In particular, zsh $ZSH_VERSION has bugs and should"])
>      AS_ECHO(["$[]0: be upgraded to zsh 4.3.4 or later."])
>    else
> @@ -488,7 +488,7 @@ fi
>  # suppresses any "Segmentation fault" message there.  '((' could
>  # trigger a bug in pdksh 5.2.14.
>  for as_var in BASH_ENV ENV MAIL MAILPATH
> -do eval test x\${$as_var+set} = xset \
> +do eval [[ \${$as_var+y} ]] \
>    && ( (unset $as_var) || exit 1) >/dev/null 2>&1 && unset $as_var || :
>  done
>  PS1='$ '
> @@ -1272,7 +1272,7 @@ fi
>  # Compute the path separator.
>  m4_defun([_AS_PATH_SEPARATOR_PREPARE],
>  [# The user is always right.
> -if test "${PATH_SEPARATOR+set}" != set; then
> +if [[ ! ${PATH_SEPARATOR+y} ]]; then
>    PATH_SEPARATOR=:
>    (PATH='/bin;/bin'; FPATH=$PATH; sh -c :) >/dev/null 2>&1 && {
>      (PATH='/bin:/bin'; FPATH=$PATH; sh -c :) >/dev/null 2>&1 ||
> diff --git a/tests/base.at b/tests/base.at
> index 63bd36e..699d146 100644
> --- a/tests/base.at
> +++ b/tests/base.at
> @@ -342,7 +342,7 @@ my_cv_variable=true
>  AC_MSG_RESULT([$my_cv_variable])
>  
>  # Ensure that the result is available at this point.
> -if test ${my_cv_variable+set} != set; then
> +if [[ ! "${my_cv_variable+y}" ]]; then
>    AC_MSG_ERROR([AC@&@&address@hidden@_CACHE_VAL did not ensure that the 
> cache variable was set])
>  fi
>  
> diff --git a/tests/m4sh.at b/tests/m4sh.at
> index df39ae7..6f68b28 100644
> --- a/tests/m4sh.at
> +++ b/tests/m4sh.at
> @@ -297,7 +297,7 @@ test $as_lineno = 9999 || AS_ERROR([bad as_lineno at 
> depth 2])
>  AS_LINENO_POP
>  test $as_lineno = 9999 || AS_ERROR([bad as_lineno at depth 1])
>  AS_LINENO_POP
> -test x${as_lineno+set} = xset && AS_ERROR([as_lineno set at depth 0])
> +test "${as_lineno+y}" && AS_ERROR([as_lineno set at depth 0])

Ahh, you kept 'test' here.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature