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

[Michael Geng]

On Tue, Aug 28, 2007 at 01:21:46PM -0700, Eric Blake-1 wrote:
> > +++ coreutils-6.9/src/ls.c  2007-08-26 19:58:20.000000000 +0200
> > @@ -76,7 +76,6 @@
> >  # define SA_RESTART 0
> >  #endif
> >  
> > -#include "system.h"
> >  #include <fnmatch.h>
> 
> Why are you deleting this include?  Without it, how do you ensure
> that <config.h> is pulled in before anything else?  If you intend for
> ls-clp.h to fill this role, then it must be included before any
> system files.  Also, are you sure you are not falling foul of
> any 'make distcheck' rules in Makefile.maint?

I need the following definitions in ls-clp.c:

1. the i18n macro _()
2. the definition of PACKAGE_BUGREPORT
3. the definition of true and false

I got everything by including system.h in ls-clp.c. Unfortunately
I had to exclude it from ls.c then because there were duplicate 
definitons. I entered this when I wrote the patch for the wc command
and at that time I was happy to get it compiled, that's all.

I'm sure there is a better solution. Maybe I have to include other
files. I agree that this has to be fixed.

> > +  Cmdline(&cmdline, argc, argv);
> 
> GNU coding standards want a space between the function
> name and open (.

Right, thanks.

> > +/* Extract the following section an process it with genparse
> > +   (see http://genparse.sourceforge.net) in order to generate a parser
> > +   for the command line arguments and a usage function for printing a
> > help 
> > +   screen. */
> > +
> > +/* genparse file starts here
> > +#include <config.h>
> > +#include "system.h"
> > +#include "ls.h"
> > +
> > +#exit_value LS_FAILURE
> 
> I know the C standard requires this, but in practice, are all
> C preprocessors tolerant of comments that contain lines
> that look like preprocessing directives but which are not?

That's potentially another drawback of embedding the genparse files in the
C sources.

> > +NONE / help                        flag    "display this help and exit"
> > +NONE / version                     flag    "output version information and 
> > exit"
> 
> It looks like one drawback of using genparse is that you lose
> the system.h magic that ensures consistency between all
> the apps with --help and --version, since you can't really
> use the preprocessor macros *_HELP_OPTION_* here.

I could imagine that this can be solved by adding the capability 
to include parameter definitions in a genparse file, i.e. include 
genparse files in other genparse files. There could be a shared 
genparse file with the parameter definitions for help and version 
which could be included by all other genparse files.

> > +Report bugs to <__STRING__(PACKAGE_BUGREPORT)>.
> 
> What happened to the TRANSLATOR comment that reminds
> them to add a second line, including the address to report
> translation bugs to?  Also, it isn't very obvious how this
> will affect xgettext extraction of strings that need
> translation.  Are you sure you haven't broken things
> for other locales?  Would the generated ls-clp.c need
> to be added to POTFILES.in, or is your intent still to
> have all translatable strings reside in ls.c?

If I understood the i18n mechanism right then the C preprocesor
is needed for the _() macros to take effect. So the genparse files
can't be translated directly, even if they are embedded in C files
because they are still inside of a comment. So I think ls-clp.c
would have to be added to POTFILES.in. 

I haven't investigated how a genparse based solution affects
i18n and I generally have very view experiance with i18n. I 
would expect that problems are caused by different partitioning 
of text. In the present version of ls the usage() function calls 
fputs() several times. The genparse version prints everything in 
1 single call to printf(). So the usage() text in the present ls.c
is split into multiple _() macros, whereas ls-clp.c uses 1 single _() 
macro for the whole help screen. Do you agree that this is the main
source of trouble? Do you see other problems? I haven't fully 
thought this through but I think I could change genparse such that 
the user can control when a new print command should start thus 
giving control of partitioning translatable text to the user.

Michael