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

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Michael Geng on 8/29/2007 2:29 PM:
> 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:

My complaint was not that you moved #include "system.h" to ls-clp.h (via
the genparse chunk), but that you forgot to put #include "ls-clp.h" first,
prior to <fnmatch.h>.  Remember, in gnulib-based projects, <config.h>
absolutely has to be included prior to any system headers, because we
provide replacement system headers (such as a replacement <stdio.h>), and
our replacements sometimes depend on the contents of <config.h> (although
we are trying to fix those cases where we can).

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

Shouldn't true and false just come from C99?  Or even the gnulib
<stdbool.h> module?  Here's a case where providing your own definition in
ls-clp.h is liable to break if you first include "system.h" (which picks
up the C99 or gnulib <stdbool.h>).

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

Not quite - xgettext is not a C preprocessor, rather it is a regular
expression matcher.  It recognizes C comments, and normally will not
extract any language comments inside them, but on the other hand, the
gettext manual recommends teaching xgettext the patterns to look for so
that translations can be grabbed from original source files rather than
from generated byproducts (ie. POTFILES.in would list getdate.y, not the
bison-generated getdate.c, if getdate has translatable strings).  Really,
the role of the C preprocessor here is to make typing gettext () shorter,
as in _(); provided that xgettext is told that _ marks a translatable string.

> 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 think that is true, unless you can teach xgettext to look inside comments.

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

Not only because of the 509-character string literal limit that Jim
mentioned, but also because gettext recommends providing no more than
about 6 or 7 lines of text to the translator at a time.  The more lines
there are to translate in one go, the harder it is for a translator to
spot the minor change embedded in those lines when all you do is edit one
word in the string.  The gettext manual talks more about this.

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

Yes, that's definitely part of the problem.  The other part is that the
_() macro only works if xgettext was able to extract the string to begin with.

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

Sounds like it would definitely be needed before coreutils could consider
switching to genparse.

By the way, thanks for your efforts in trying to improve all of this.
Even if Jim doesn't accept your code, it is making genparse better, and it
is finding areas in coreutils that could use improvement regardless of how
option-parsing code is generated.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG1i3B84KuGfSFAYARAm4PAJ97DH6kdl19fNw/+FotSFVUBQx7ngCg16G8
KXV7YZax8ouaeUPkwKnIY8w=
=Zjc2
-----END PGP SIGNATURE-----