On 9 April 2015 at 15:13, Eric Blake <ebl...@redhat.com> 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 :)
