[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Command line parsing of wc with genparse
From: |
Michael Geng |
Subject: |
Re: [PATCH] Command line parsing of wc with genparse |
Date: |
Mon, 16 Jul 2007 21:55:47 +0200 |
User-agent: |
Mutt/1.5.9i |
On Mon, Jul 16, 2007 at 10:02:13AM +0200, Jim Meyering wrote:
> 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"?
I did this now - and I found a bug: wc -l doesn't work because I didn't assign
the print_lines variable. That's not a bug in genparse but in my hand written
code in wc.c. But after fixing this make check still fails because the usage()
function doesn't print a proper PACKAGE_BUGREPORT as you also found.
> 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.
I will need some time to think about all the issues you mentioned and
find solutions. I appreciate your constructive commments!
Michael