[illumos-Developer] Webrev for bug 844: m4 should say something useful...

[Garrett D'Amore]

On Sun, 2011-04-03 at 18:35 -0500, Gary Mills wrote:
> On Sun, Apr 03, 2011 at 02:58:24PM -0700, Garrett D'Amore wrote:
> > On Sun, 2011-04-03 at 10:40 -0500, Gary Mills wrote:
> > > 
> > > Okay, there's a new webrev at:
> > > 
> > >     http://cr.illumos.org/view/k9zwpc1c/illumos844-1/
> > > 
> > > that uses the string you recommended.  Here's how it looks now:
> > > 
> > >     $ usr/src/cmd/sgs/m4/i386/m4 sample.m4
> > >     
> > >     usr/src/cmd/sgs/m4/i386/m4:sample.m4:2 cannot open file: No such file or directory
> > >     
> > >     include(foobar.m4)
> > >     $ env LC_MESSAGES=fr_CA.UTF-8 usr/src/cmd/sgs/m4/i386/m4 samp>
> > >     
> > >     usr/src/cmd/sgs/m4/i386/m4:sample.m4:2 impossible d'ouvrir le fichier : Ce fichier ou ce répertoire n'existe pas
> > > 
> > >     include(foobar.m4)
> > > 
> > > It is translated now.  Because of the `\n' in the new string, there is
> > > an extra newline in the error message.  I'm not concerned about that.
> > 
> > Huh?  There should only be one new line.
> 
> It's because of the layering.  The code itself calls error(),
> error2(), or now errorf() to report errors.  All of them take a string
> that has no trailing newline as the first argument.  error2() inserts
> an integer whereas errorf() inserts a string before calling error().
> Finally, this function prints to stderr, appending a newline to the
> complete string.
> 
> > > Is this change ready to go now?
> > 
> > I'd like to understand what you meant by an "extra" newline.  The code
> > looks good to me, though honestly I'd have simply opted for perror(3c)
> > instead of inventing a new function. E.g.
> > 
> > 	if (fopen(...) == NULL) {
> > 		perror(gettext("cannot open file"));
> > 		...
> > 	}
> > 
> > perror looks up the errno for you, and also dumps the result to stderr.
> 
> That would violate the layering.  The real problem is that
> 
>     "cannot open file: %s\n"
> 
> is already in the message catalog, but the same string without the
> trailing newline seems not to be there.  I don't know what the
> convention is, but all the error strings in m4 have no trailing
> newline.
> 
> > If you don't to make the switch to use perror, that's OK, its just not
> > quite as simple/clean as if you use perror().
> 
> Okay, what's my next step to get this fix integrated?

We're still in review feedback.

Looking at the above, I see, I didn't realize that error() was dumping
more than just the error message.

I'd like request stripping the leading newline.  I realize that it will
cause a hit to translation, but I'm OK with the message catalog needing
an update for this case.  I prefer that to incorrectly formatting the
message with an extra newline.  (I realize this is a nit, but let's get
it right.)

	- Garrett