Hi Eric,
Thanks for the feedback.
On 22 Nov 2011, at 03:05, Eric Blake wrote:
> On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
>> Until now, libtool sources have used braced variable names
>> seemingly at random! Almost always the braces are just noise, so
>> remove all the unnecessary ones.
>> * cfg.mk (sc_useless_braces_in_variable_derefs): New syntax
>> check rule to ensure we only reintroduce braced variable
>> dereferences if they are followed by a valid variable name
>> character.
>> build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
>> build-aux/ltmain.m4sh, build-aux/options-parser, configure.ac,
>> libltdl/configure.ac, m4/libtool.m4, m4/ltdl.m4,
>> m4/ltoptions.m4, tests/defs.m4sh, tests/demo-nopic.test,
>> tests/depdemo/configure.ac, tests/flags.at, tests/link.test,
>> tests/objectlist.test, tests/quote.test, tests/static.at: Remove
>> spurious braces.
>>
>> +++ b/build-aux/ltmain.m4sh
>
>> @@ -152,7 +152,7 @@ exec_cmd=
>> # Append VALUE to the end of shell variable VAR.
>> func_append ()
>> {
>> - eval "${1}=\$${1}\${2}"
>> + eval "$1=\$$1\$2"
>
> Not necessarily correct. One of the reasons for using ${1} in any m4
> code that comprises the m4_define() definition of a macro is that $1 is
> replaced by an argument by m4 in expanding the macro, while ${1} is
> preserved unchanged through m4 expansion to be saved for the resulting
> shell code. I fear that your global search-and-replace may have been
> too eager in m4-related files, but haven't read it closely to check for
> sure; even if it didn't, the stylistic convention of ${1} for shell
> expansion vs. $1 for m4 expansion made the file slightly easier to maintain.
I went back and forth on this myself.
In the end, I didn't want to exclude .m4sh tests from the syntax check
because there's a ton of our messiest shell code in those files, which
is most of the reason for adding the new syntax-checks in the first place.
While it's true that the shell braces do prevent M4 from substituting its
own positional parameters, the vast majority of uses in our .m4sh code are
wrapped in M4SH_VERBATIM([[...]]), so they are left unexpanded for the
shell even with the braces removed. In the handful of cases where we had
previously saved positional parameters from being prematurely expanded by
M4 using braces, again I'm reluctant to exclude the whole file from the
syntax-check, so I used quadrigraphs instead to keep all the tools happy :)
Cheers,
--
Gary V. Vaughan (gary AT gnu DOT org)
