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 (g...@gnu.org)