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

[Eric Blake-1]

> 2. ls.c depends ls-clp.h (the generated parser)
>    ls-clp.h depends on ls.gp (the genparse file)
>    ls.gp depends on ls.c because ls.gp is embedded as a comment in ls.c
>    -> There is a circular dependency!

That seems wrong to me.  Isn't it really:

ls$(EXEEXT) directly depends on ls.o and ls-clp.o
ls.o directly depends on ls.c and ls-clp.h
ls-clp.o directly depends on ls-clp.c and ls-clp.h
ls-clp.c directly depends on ls.gp
ls-clp.h directly depends on ls.gp
ls.gp directly depends on ls.c

No cycle there, even though ls.c is an indirect
dependency of ls$(EXEEXT) through more than one
leg of the transitive closure.

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

> +  Cmdline(&cmdline, argc, argv);

GNU coding standards want a space between the function
name and open (.

> -     case 'w':
> -       {
> -         unsigned long int tmp_ulong;
> -         if (xstrtoul (optarg, NULL, 0, &tmp_ulong, NULL) != LONGINT_OK
> -             || ! (0 < tmp_ulong && tmp_ulong <= SIZE_MAX))
> -           error (LS_FAILURE, 0, _("invalid line width: %s"),
> -                  quotearg (optarg));

[Side issue, not a problem with your patch, but your
patch highlights the problem - why are we using plain
error instead of xstrtol_error?  It means this particular
error message loses some quality.]

> +/* 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?

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

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

-- 
Eric Blake

-- 
View this message in context: 
http://www.nabble.com/-PATCH--Command-line-parsing-of-ls-with-genparse-tf4343050.html#a12375174
Sent from the Gnu - Coreutils - Discuss mailing list archive at Nabble.com.