Re: [PATCH] Miscellaneous code readability improvements.

[Joel E. Denny]

On Wed, 12 Aug 2009, Akim Demaille wrote:

> > Subject: [PATCH] Miscellaneous code readability improvements.
> 
> Much better, indeed!

Thanks!

> >  "'"|"\n" {
> > -    STRING_GROW;
> >    STRING_FINISH;
> >    loc->start = token_start;
> > -    val->character = last_string[1];
> > +    val->character = last_string[0];
> >    {
> >      /* FIXME: Eventually, make these errors.  */
> > -      size_t length = strlen (last_string);
> > -      if (length < 3)
> > -        warn_at (*loc, _("empty character literal"));
> > -      else if (length > 3)
> > +      if (last_string[0] == '\0')
> > +        {
> > +          warn_at (*loc, _("empty character literal"));
> > +          /* '\0' seems dangerous even if we are about to complain.  */
> > +          val->character = '\'';
> > +        }
> 
> Aren't you preventing the use of the character '\0' itself?  While dubious in
> the strings, it should probably be valid in the characters.  But then of
> course we need to handle the case of EOF :(

POSIX says:

  The application shall ensure that the NUL character is not used in 
  grammar rules or literals.

(I don't know what it means to have the NUL character in grammar rules.  
Maybe it means the NUL character in a literal in a grammar rule, but 
that's redundant to say.  Maybe it means the actions.  Anyway....)

Moreover, Bison already complains about NUL in a literal and discards it, 
so my code above never sees the user's '\0'.

But this brings up another issue: the quoting in that complaint has too 
much escaping:

  tmp.y:2.9-10: invalid null character: `\\0'

The following patch seems to be the obvious fix, but the last case bothers 
me.  What if the user's "\" is followed by a special character that really 
needs to be converted into an escape sequence before being printed?

diff --git a/src/scan-gram.l b/src/scan-gram.l
index 73eb3e8..6a65bd5 100644
--- a/src/scan-gram.l
+++ b/src/scan-gram.l
@@ -522,9 +522,9 @@ splice       (\\[ \f\t\v]*\n)*
   \\[0-7]{1,3} {
     unsigned long int c = strtoul (yytext + 1, NULL, 8);
     if (UCHAR_MAX < c)
-      complain_at (*loc, _("invalid escape sequence: %s"), quote (yytext));
+      complain_at (*loc, _("invalid escape sequence: `%s'"), yytext);
     else if (! c)
-      complain_at (*loc, _("invalid null character: %s"), quote (yytext));
+      complain_at (*loc, _("invalid null character: `%s'"), yytext);
     else
       obstack_1grow (&obstack_for_string, c);
   }
@@ -533,9 +533,9 @@ splice       (\\[ \f\t\v]*\n)*
     verify (UCHAR_MAX < ULONG_MAX);
     unsigned long int c = strtoul (yytext + 2, NULL, 16);
     if (UCHAR_MAX < c)
-      complain_at (*loc, _("invalid escape sequence: %s"), quote (yytext));
+      complain_at (*loc, _("invalid escape sequence: `%s'"), yytext);
     else if (! c)
-      complain_at (*loc, _("invalid null character: %s"), quote (yytext));
+      complain_at (*loc, _("invalid null character: `%s'"), yytext);
     else
       obstack_1grow (&obstack_for_string, c);
   }
@@ -554,14 +554,14 @@ splice     (\\[ \f\t\v]*\n)*
   \\(u|U[0-9abcdefABCDEF]{4})[0-9abcdefABCDEF]{4} {
     int c = convert_ucn_to_byte (yytext);
     if (c < 0)
-      complain_at (*loc, _("invalid escape sequence: %s"), quote (yytext));
+      complain_at (*loc, _("invalid escape sequence: `%s'"), yytext);
     else if (! c)
-      complain_at (*loc, _("invalid null character: %s"), quote (yytext));
+      complain_at (*loc, _("invalid null character: `%s'"), yytext);
     else
       obstack_1grow (&obstack_for_string, c);
   }
   \\(.|\n)     {
-    complain_at (*loc, _("unrecognized escape sequence: %s"), quote (yytext));
+    complain_at (*loc, _("unrecognized escape sequence: `%s'"), yytext);
     STRING_GROW;
   }
 }