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

[Michael Geng]

On Mon, Jul 16, 2007 at 09:55:47PM +0200, Michael Geng wrote:
> 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.

I changed it to PROGRAM_NAME. Thanks.

> >     > 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

Again I released a new version of genparse (0.6.7) which now allows to include
string macros into the usage() function. So genparse is now able to use the 
PACKAGE_BUGREPORT macro.

> > 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.

I 'm attaching an updated patch which passes make check. You need genpare 
version
0.6.7 in your path in order to compile coreutils after you applied the patch.

> > Make sure valgrind wc ... shows no leaks, too.
> > In particular, test that with the --files0-from=F option.

valgrind wc --files0-from=F tells me something about 3 not-freed blocks.
But it gives exactly the same message if I'm executing valgrind wc 
--files0-from=F
from a coreutils build from the original sources of version 6.9. Is there a 
known memory leak with the --files0-from=F option of wc?

I tried vagrind wc ... with some other options but it didn't report any errors
except for the --files0-from option.

> > Is there any significant difference in performance (I doubt it) or binary 
> > size?

I inspected the wc which was generated using genparse with the original one and
found that the .text segment increased by 16 bytes. BTW, how can I build the
coreutils without debug information? configure CFLAGS="-O2 -g0" doesn't
remove debug information completely. Would you build coreutils differently for 
comparing binary file sizes? How exactly would you compare binary file sizes? 
I think comparing the .text segment size is not enough.

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

Presently genparse is able to handle int, float, char, string or flag types.
No unsigned int for example. That's another extension which I will have to
add.

> > 
> > 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 :)

That's probably a good path to move on, but now that I started with the wc 
command I wanted to get this one working first. 

> > 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.

Could be done this way.

Michael

genparse-wc-2.patch
Description: Text document