[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Use getopt.m4sh to generate libtool option parser.
From: |
Ralf Wildenhues |
Subject: |
Re: [PATCH] Use getopt.m4sh to generate libtool option parser. |
Date: |
Sat, 12 Jun 2010 12:45:45 +0200 |
User-agent: |
Mutt/1.5.20 (2009-10-28) |
* Gary V. Vaughan wrote on Sat, Jun 12, 2010 at 12:34:04PM CEST:
> On 12 Jun 2010, at 15:48, Ralf Wildenhues wrote:
> > 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.
Well, these are two orthogonal issues. But the leading space is still
not a problem. :-)
> > 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?
No. I simply wasn't aware that the inconsistency had a reason other
than oversight. Never mind then.
> 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.
Not needed, thanks.
> > 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.
Point well taken. Oversight of mine in reviewing.
> > 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.
Ah. Newline is fine with me then.
> > 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'm fine with not using XSI for all other scripts; the func_opt_split
could be trivial forwarders for the sed scripts there; that would avoid
having to beef up infrastructure here, IIUC. But the libtool command
line parsing really was one of the time critical issues in user builds,
and --mode and --tag are pervasive, so this would be a real regression.
Thanks,
Ralf
- [PATCH] Use getopt.m4sh to generate libtool option parser., Gary V. Vaughan, 2010/06/11
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser., Ralf Wildenhues, 2010/06/12
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser., Gary V. Vaughan, 2010/06/12
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser.,
Ralf Wildenhues <=
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser., Gary V. Vaughan, 2010/06/22
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser., Ralf Wildenhues, 2010/06/22
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser., Gary V. Vaughan, 2010/06/22
- [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements., Gary V. Vaughan, 2010/06/22
- Re: [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements., Gary V. Vaughan, 2010/06/23
- Re: [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements., Ralf Wildenhues, 2010/06/26
- Re: [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements., Gary V. Vaughan, 2010/06/27
- Re: [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements., Ralf Wildenhues, 2010/06/26
- Re: [PATCH] getopt.m4sh generated libtool option parser, and XSI improvements., Gary V. Vaughan, 2010/06/27
- Re: [PATCH] Use getopt.m4sh to generate libtool option parser., Ralf Wildenhues, 2010/06/22