bug-coreutils
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]