Re: [PATCH] Command line parsing of wc with genparse

[Jim Meyering]

address@hidden (Michael Geng) wrote:
> I released version 0.6.6 of genparse which supports
> internationalization now. A link to the download page
> and to the updated documentation can be accessed from
> the genparse project page http://genparse.sourceforge.net/.
>
> I also prepared a patch which demonstrates how genparse
> could be used for parsing the command line arguments of
> the wc command. The patch is relative to the coreutils version
> 6.9. In order to compile the coreutils after you applied
> the patch you will need genparse version 0.6.6 in your path.
>
> Does this approach appear reasonable to you? What else should
> be improved on genparse?

I took a quick look only at the patch (not at the code genparse generates)
and spotted some minor problems:

    > +  if (cmdline.version)
    > +    {
    > +      version_etc (stdout, program_name, GNU_PACKAGE, VERSION, AUTHORS,

    Here, it should be PROGRAM_NAME, not program_name.
    ...
    > diff -u -N -r coreutils-6.9.orig/src/wc.gp coreutils-6.9/src/wc.gp
    > --- coreutils-6.9.orig/src/wc.gp  1970-01-01 01:00:00.000000000 +0100
    > +++ coreutils-6.9/src/wc.gp       2007-07-10 18:17:33.000000000 +0200
    > @@ -0,0 +1,24 @@
    ...
    > +Report bugs to <address@hidden>.
    > +#usage_end

    s/fileutils/coreutils/
    or, perhaps better: PACKAGE_BUGREPORT

Did you ensure that wc still passes "make check"?
Make sure valgrind wc ... shows no leaks, too.
In particular, test that with the --files0-from=F option.
Is there any significant difference in performance (I doubt it) or binary size?

I suggest you also adapt a program that takes integer arguments.
Can genparse handle arguments of all different types, from int
through uintmax_t?

tail looks like a good candidate.  It also had an optional argument:
--follow[={name|descriptor}]

There are quite a few programs in the coreutils that have unusual
argument-parsing constraints, so it's hard to point to a small subset
and say "if you adapt just these few", then that should be proof enough.
I'm afraid that if you're interested in having coreutils convert, you'll
have to do most of the work, and then demonstrate that there's no
resulting regression (and presumably cleaner code).

For now, let's start with tail, and maybe "ls" if you're ambitious :)

Another detail:
IMHO, the contents of your wc.gp file should reside in wc.c.
Might as well leave it in a delimited comment in place of the usage
function you're eliminating, then have a makefile rule use sed or awk
to extract it into wc.gp.