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

[Ralf Wildenhues]

Hi Gary,

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.

> +      *)  func_stripname_result=`$ECHO "${3}" | $SED "s%^${1}%%; 
> s%${2}\$%%"`;;
> +    esac
> +} # func_stripname may be replaced by XSI optimised implementation
>  
> -# Generated shell functions inserted here.


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

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

> +# _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.

> +$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.  And exiting from that should
be done with AS_EXIT or better AC_MSG_ERROR.

> +rm -f "$cfgfile.tmp"])

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)?

Thanks,
Ralf