Re: [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements.

[Gary V. Vaughan]

Hallo Ralf,

Thanks again for the review.

On 27 Jun 2010, at 04:26, Ralf Wildenhues wrote:
> another partial review:
> 
> * Gary V. Vaughan wrote on Tue, Jun 22, 2010 at 10:23:39PM CEST:
>> --- a/libltdl/config/general.m4sh
>> +++ b/libltdl/config/general.m4sh
> 
> 
>> +func_stripname ()
>> +{ 
> 
> Trailing whitespace.
> 
>> +    case ${2} in
>> +      .*) func_stripname_result=`$ECHO "${3}" | $SED "s%^${1}%%; 
>> s%\\\\${2}\$%%"`;; 
> 
> Likewise.

I fixed those before I pushed.

>> --- a/libltdl/config/getopt.m4sh
>> +++ b/libltdl/config/getopt.m4sh
>> @@ -619,6 +610,32 @@ func_missing_arg ()
>>     exit_cmd=exit
>> }
>> 
>> +
>> +# func_split_short_opt shortopt
>> +# Set func_split_short_opt_name and func_split_short_opt_arg shell
>> +# variables after splitting SHORTOPT at the `=' sign.
>> +func_split_short_opt ()
>> +{
>> +    my_sed_short_opt='1s/^\(..\).*$/\1/;q'
>> +    my_sed_short_rest='1s/^..\(.*\)$/\1/;q'
>> +
>> +    func_split_short_opt_name=`$ECHO "$1" | $SED "$my_sed_short_opt"`
>> +    func_split_short_opt_arg=`$ECHO "$1" | $SED "$my_sed_short_arg"`
>> +} # func_split_short_opt may be replaced by XSI optimised implementation
> 
> You don't actually replace it by an XSI implementation though, do you?
> TODO item for next patch?

Correct, same as prior to the patch.

I'll propose an XSI func_split_short_opt presently.

>> --- a/libltdl/m4/libtool.m4
>> +++ b/libltdl/m4/libtool.m4
>> @@ -748,15 +748,12 @@ _LT_EOF
>>   # if finds mixed CR/LF and LF-only lines.  Since sed operates in
>>   # text mode, it properly converts lines to CR/LF.  This bash problem
>>   # is reportedly fixed, but why not run on old versions too?
>> -  sed '/^# Generated shell functions inserted here/q' "$ltmain" >> 
>> "$cfgfile" \
>> -    || (rm -f "$cfgfile"; exit 1)
>> +  sed '$q' "$ltmain" >> "$cfgfile" \
>> +     || (rm -f "$cfgfile"; exit 1)
> 
> sed '$q' is equivalent to cat, right?

Yep, factored from the prepatch code and documented by the previous comment.

>> +# _LT_PROG_XSI_REPLACE (FUNCNAME, REPLACEMENT-BODY)
>> +# -------------------------------------------------
>> +# In `$cfgfile', look for function FUNCNAME delimited by `^FUNCNAME ()$' and
>> +# '^} FUNCNAME ', and replace its body with REPLACEMENT-BODY.
>> +m4_defun([_LT_PROG_XSI_REPLACE],
>> +[dnl {
>> +sed -i .tmp -e '/^$1 ()$/,/^} # $1 /c\
> 
> -i is GNU sed-specific, and -i with space before the backup suffix is
> probably only portable to one specific version of GNU sed.  You need to
> use a temporary file and mv.

Ugh.  My bad.  Pushing a fix right now.

>> +$1 ()\
>> +{\
>> +m4_bpatsubst([$2], [$], [\\])
>> +} # XSI $1 implementation' "$cfgfile" \
>> +  || (mv -f "$cfgfile.tmp" "$cfgfile"; exit 1)
> 
> What is that exit supposed to do?  An exit in a subshell won't exit the
> configure script, so this will be a no-op.

D'oh, thanks. I factored blindly from the prepatch code.

>  And exiting from that should
> be done with AS_EXIT or better AC_MSG_ERROR.

On reflection, bailing out seems like overkill here.  I think a
warning that the XSI function replacement failed is more appropriate
in this case, since everything will still work even if the replacement
process fails.

> Have you tested the patch on both a system with an XSI shell (such as
> bash or ksh) and one without (such as Solaris setting CONFIG_SHELL)?

No, I was working on all this stuff because all the Unix machines I
usually have access to were offline.  I tested with and without XSI
replacements on Mac OS X bash.  Those machines are back now, so I'll
double check later today.

Cheers,
-- 
Gary V. Vaughan (address@hidden)