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

[Bernhard Reutner-Fischer]

On 9 April 2015 at 15:13, Eric Blake <address@hidden> wrote:
> 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).

ok.
>
>>
>> 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.

As you prefer.

>> --- 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 i did then we should add a testcase for this case for sure. I don't
immediately see
potential problems with that though, IIRC m4 should expand $digit just
fine even after
having seen an escaped '[', no?

> 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.

m4 is great ;)

>
>>  ])
>>
>>  # 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.

the nesting is ok, it's emitted properly in the configure files i diffed.
Fine to drop this hunk if you prefer.
>
>>       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.

I did not want to munge too many changes into this one patch. TODO.

>
>>       then :; else
>>          ac_cv_exeext=`expr "$ac_file" : ['[^.]*\(\..*\)']`
>>       fi

>> 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.

${x=$default} is even better, agree.
Given that this was even in SUSv3 it should be reasonable safe to use
this by now.
[I would just bundle a sane fallback shell with autoconf and nuke all
the legacy support right away, but well]

>> --- 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.

yea, that was on purpose for consistency with the 'test' above :)