Re: [PATCH] Use getopt.m4sh to generate libtool option parser.

[Gary V. Vaughan]

On 12 Jun 2010, at 15:48, Ralf Wildenhues wrote:
> Hi Gary,

Hallo Ralf,

Thanks again for the review.

> * Gary V. Vaughan wrote on Fri, Jun 11, 2010 at 07:22:52PM CEST:
>> Okay to push?
> 
>> * libltdl/config/ltmain.m4sh: Replace hand written shell code
>> with a call to M4SH_GETOPTS. Rename some option variables in
>> the client code to match the generated option parser settings.
>> * libltdl/config/general.m4sh (func_echo, func_error)
>> (func_warning): Use $opt_mode instead of obsoleted $mode in
>> message leader.
> 
> Some nits:
> 
> The variable preserve_args is not properly initialized any more,
> so it could pick up junk from the environment.

Okay. I did that to avoid the initial space, otherwise
${preserve_args+.... } is set and expands to the part with the
space.  But since a spurious leading space is better that picking
up garbage from the environment, that's an obvious change.

> The addition of
> --debug to preserve_args is moved out of the parsing loop, unlike
> the other preserved args, which is inconsistent.

That's because M4SH_GETOPTS adds a consistent (wrt other clients of
M4SH_GETOPTS) --debug|-x -> $opt_debug case branch to the option
parsing loop automatically, and the preserved_args handling is bespoke
to ltmain.m4sh.  Does the change of argument ordering in the case of
--debug really matter?  If it could cause problems, then I'll remove
automatic --debug processing from M4SH_GETOPTS, and will have to add
it back manually in each of our other scripts.

> I don't think
> you need to take care about the leading extra space in preserve_args,
> IOW, I think you can simplify all those
>  preserve_args="${preserve_args+$preserve_args }$opt"
> 
> to
>  preserve_args="$preserve_args $opt"

okay.

> I'm not sure why some of the variable renamings are done, e.g.,
> opt_preserve_dup_deps, or execute_dlfiles, or mode.  These could easily
> be done in a separate patch (and then, they would be trivial to verify,
> and maybe easier to understand why they are needed).

When M4SH_GETOPT generates the code for a given command line option
that you specify to it, the associated variable name is generated by
stripping the leading `--' from the long option name (or short option
if there is no long option, but that is irrelevant for now), converting
non alphanumeric characters to `_' and adding a leading `opt_'.  So, if
the long option name is --preserve-dup-deps, then M4SH_GETOPTS generates
code that fills opt_preserve_dup_deps appropriately, and that is less of
an impact than changing the option name to --duplicate-deps to retain
opt_duplicate_deps. Similarly for --mode to opt_mode. This seems more
consistent to me (whatever was specified to the script as a command line
option can be found in an associated opt_option_name variable) and is
an improvement over the arbitrary names we were using until now.

Of course we could add an extra assignment in the custom code, but that
seems pointless to me, and would generate:

        --mode)
                        test $# = 0 && func_missing_arg $opt && break
                        optarg="$1"
                        opt_mode="$optarg"
                        mode="$opt_mode"
                        ...

> Why do you now separate entries in opt_dlopen (formerly execute_dlfiles)
> with newlines, rather than, as formerly, with spaces?

The M4SH_GETOPTS ';' specifier does newline separation in additive
options for more flexibility (and is required to support multiword
additive options such as --header='Reply-To: address@hidden' in
announce-gen).

We could add another specifier for space separated additive options,
but that seems like busy work to me when the existing ';' works very
well, and is more general.

> The indentation in the generated libtool script is a bit awkward,
> but I guess there's little we can do about it with this scheme.

I did try to fix that by injecting tabs after each newline, but
couldn't get the m4 quoting quite right.  Maybe when we move it over
to autoconf I'll be able to get some help in figuring out where
I've overquoted.

> The handling of --dlopen=..., --mode=..., --tag=..., now increased by
> two forks and one exec per such flag on systems with decent shells, and
> it doesn't use safe $ECHO any more even on systems with indecent shells,
> because you don't use func_opt_split any more.  My guess is that this
> would increase libtool execution time for typical compile commands by
> a noticeable amount, since we got them down to about half a dozen forks.
> Please fix this.

That's quite an invasive change, since the core of the M4SH_GETOPTS
generated code will then rely on the XSI functions you add to libtool at
configure time.  I suppose we need to put them in, say,
libltd/config/xsi.m4sh where libtoolize, commit, announce-gen,
mailnotify and any future m4sh scripts can reap the benefits.  This
in turn means we need to move it into a separate macro to contribute
back to Autoconf when we move getopt.m4sh and supporting code.

I'll work on this first as a separate patch, and then I'll resubmit
this one reworked to use it, along with undoing the leading space
fix; but, I think it would be wrong to change the other things you
mention, for the reasons explained above.  It'll be another week or
two before I have time to work on Libtool some more though.

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