Re: Fix %error-verbose for conflicts resolved by %nonassoc.

[Joel E. Denny]

On Tue, 25 Aug 2009, Joel E. Denny wrote:

> On Tue, 25 Aug 2009, Joel E. Denny wrote:
> 
> > I feel this area is a bit too subtle to include this change 
> > in branch-2.4.2.
> 
> At least I got that right.
> 
> As for the rest, the following patch hopefully fixes it.

Nope, so I pushed the patch below to master and a similar one to 
branch-2.5.

My confusion has been as follows.  The comments in the source code said 
that a zero in yytable indicates that a default action should be used.  
However, the code in the skeletons checked for zero as if it meant syntax 
error instead.  In the previous patch, I just assumed that the comments 
were out of date and that the skeletons were right because they work 
correctly.  But that was wrong.  The issue is more subtle.

I finally decided to thoroughly investigate the code in tables.c to see 
how the tables are actually being computed.  It turns out that the 
comments were right but incomplete.  The zero check in the skeletons is 
always false and thus misleading but harmless.  I tested this conclusion 
by temporarily adding an assertion to yytable_value_is_error in all four 
skeletons.  maintainer-check-valgrind still passed.

The patch below cleans up the documentation and the misleading code.

> diff --git a/data/lalr1.java b/data/lalr1.java

> -                  && yycheck_[x + yyn] != yytable_ninf_)
> +                  && !yy_table_value_is_error_ (yycheck_[x + yyn]))

> -                      && yycheck_[x + yyn] != yytable_ninf_)
> +                      && !yy_table_value_is_error_ (yycheck_[x + yyn]))

Ugh.  That's supposed to be yytable_ not yycheck_.  It's fixed below.

Sorry for all the noise.

>From aa0cb40d61cda5bfa9d325a45735439cbbd06327 Mon Sep 17 00:00:00 2001
From: Joel E. Denny <address@hidden>
Date: Wed, 26 Aug 2009 02:40:38 -0400
Subject: [PATCH] Actually handle the yytable zero value correctly this time.

* data/bison.m4 (b4_integral_parser_tables_map): Don't mention
zero values in the YYTABLE comments.
* data/glr.c (yytable_value_is_error): Don't check for zero
value.
* data/lalr1.cc (yy_table_value_is_error_): Likewise.
* data/yacc.c (yytable_value_is_error): Likewise.
* data/lalr1.java (yy_table_value_is_error_): Likewise.
(yysyntax_error): Fix typo in code: use yytable_ not yycheck_.
* src/tables.h: In header comments, explain why it's useless to
check for a zero value in yytable.
---
 ChangeLog        |   14 ++
 data/bison.m4    |    2 +-
 data/glr.c       |    3 +-
 data/lalr1.cc    |    2 +-
 data/lalr1.java  |    6 +-
 data/yacc.c      |    3 +-
 src/parse-gram.c |  633 +++++++++++++++++++++++++++---------------------------
 src/parse-gram.h |    6 +-
 src/tables.c     |    2 +-
 src/tables.h     |   14 +-
 10 files changed, 353 insertions(+), 332 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 22a2a3c..89f863f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2009-08-26  Joel E. Denny  <address@hidden>
+
+       Actually handle the yytable zero value correctly this time.
+       * data/bison.m4 (b4_integral_parser_tables_map): Don't mention
+       zero values in the YYTABLE comments.
+       * data/glr.c (yytable_value_is_error): Don't check for zero
+       value.
+       * data/lalr1.cc (yy_table_value_is_error_): Likewise.
+       * data/yacc.c (yytable_value_is_error): Likewise.
+       * data/lalr1.java (yy_table_value_is_error_): Likewise.
+       (yysyntax_error): Fix typo in code: use yytable_ not yycheck_.
+       * src/tables.h: In header comments, explain why it's useless to
+       check for a zero value in yytable.
+
 2009-08-25  Joel E. Denny  <address@hidden>
 
        More fixes related to last two patches.
diff --git a/data/bison.m4 b/data/bison.m4
index ef8f778..0928721 100644
--- a/data/bison.m4
+++ b/data/bison.m4
@@ -266,7 +266,7 @@ $1([defgoto], [b4_defgoto], [[YYDEFGOTO[NTERM-NUM].]])
 $1([table], [b4_table],
    [[YYTABLE[YYPACT[STATE-NUM]].  What to do in state STATE-NUM.  If
 positive, shift that token.  If negative, reduce the rule which
-number is the opposite.  If zero or YYTABLE_NINF, syntax error.]])
+number is the opposite.  If YYTABLE_NINF, syntax error.]])
 
 $1([check], [b4_check])
 
diff --git a/data/glr.c b/data/glr.c
index 8d37070..6f1f7ee 100644
--- a/data/glr.c
+++ b/data/glr.c
@@ -972,8 +972,7 @@ yydefaultAction (yyStateNum yystate)
 }
 
 #define yytable_value_is_error(yytable_value) \
-  (]b4_table_value_equals([[table]], [[yytable_value]], [b4_table_ninf])[ \
-   || ]b4_table_value_equals([[table]], [[yytable_value]], [[0]])[)
+  ]b4_table_value_equals([[table]], [[yytable_value]], [b4_table_ninf])[
 
 /** Set *YYACTION to the action to take in YYSTATE on seeing YYTOKEN.
  *  Result R means
diff --git a/data/lalr1.cc b/data/lalr1.cc
index 36c8d58..8e79333 100644
--- a/data/lalr1.cc
+++ b/data/lalr1.cc
@@ -668,7 +668,7 @@ b4_percent_code_get[]dnl
   inline bool
   ]b4_parser_class_name[::yy_table_value_is_error_ (int yyvalue)
   {
-    return yyvalue == 0 || yyvalue == yytable_ninf_;
+    return yyvalue == yytable_ninf_;
   }
 
   int
diff --git a/data/lalr1.java b/data/lalr1.java
index 5f0cfd7..6637c3a 100644
--- a/data/lalr1.java
+++ b/data/lalr1.java
@@ -745,7 +745,7 @@ m4_popdef([b4_at_dollar])])dnl
             int count = 0;
             for (int x = yyxbegin; x < yyxend; ++x)
               if (yycheck_[x + yyn] == x && x != yyterror_
-                  && !yy_table_value_is_error_ (yycheck_[x + yyn]))
+                  && !yy_table_value_is_error_ (yytable_[x + yyn]))
                 ++count;
 
             // FIXME: This method of building the message is not compatible
@@ -757,7 +757,7 @@ m4_popdef([b4_at_dollar])])dnl
                 count = 0;
                 for (int x = yyxbegin; x < yyxend; ++x)
                   if (yycheck_[x + yyn] == x && x != yyterror_
-                      && !yy_table_value_is_error_ (yycheck_[x + yyn]))
+                      && !yy_table_value_is_error_ (yytable_[x + yyn]))
                     {
                       res.append (count++ == 0 ? ", expecting " : " or ");
                       res.append (yytnamerr_ (yytname_[x]));
@@ -785,7 +785,7 @@ m4_popdef([b4_at_dollar])])dnl
    */
   private static boolean yy_table_value_is_error_ (int yyvalue)
   {
-    return yyvalue == 0 || yyvalue == yytable_ninf_;
+    return yyvalue == yytable_ninf_;
   }
 
   private static final ]b4_int_type_for([b4_pact])[ yypact_ninf_ = 
]b4_pact_ninf[;
diff --git a/data/yacc.c b/data/yacc.c
index 505b09e..163ecfe 100644
--- a/data/yacc.c
+++ b/data/yacc.c
@@ -531,8 +531,7 @@ static const ]b4_int_type_for([b4_toknum])[ yytoknum[] =
 #define YYTABLE_NINF ]b4_table_ninf[
 
 #define yytable_value_is_error(yytable_value) \
-  (]b4_table_value_equals([[table]], [[yytable_value]], [b4_table_ninf])[ \
-  || ]b4_table_value_equals([[table]], [[yytable_value]], [[0]])[)
+  ]b4_table_value_equals([[table]], [[yytable_value]], [b4_table_ninf])[
 
 ]b4_parser_tables_define[
 
diff --git a/src/tables.c b/src/tables.c
index f6cc073..d1f7e7d 100644
--- a/src/tables.c
+++ b/src/tables.c
@@ -126,7 +126,7 @@ static int table_size = 32768;
 base_number *table;
 base_number *check;
 /* The value used in TABLE to denote explicit syntax errors
-   (%nonassoc), a negative infinite.  First defaults to ACTION_NUMBER_MININUM,
+   (%nonassoc), a negative infinite.  First defaults to ACTION_NUMBER_MINIMUM,
    but in order to keep small tables, renumbered as TABLE_ERROR, which
    is the smallest (non error) value minus 1.  */
 base_number table_ninf = 0;
diff --git a/src/tables.h b/src/tables.h
index 9b52f09..02381e2 100644
--- a/src/tables.h
+++ b/src/tables.h
@@ -48,7 +48,7 @@
    YYFINAL = the state number of the termination state.
 
    YYTABLE = a vector filled with portions for different uses, found
-   via YYPACT and YYPGOTO.
+   via YYPACT and YYPGOTO, described below.
 
    YYLAST ( = high) the number of the last element of YYTABLE, i.e.,
    sizeof (YYTABLE) - 1.
@@ -87,7 +87,7 @@
 
    If the value is negative, it is minus a rule number to reduce by.
 
-   If the value is zero or YYTABLE_NINF, it's a syntax error.
+   If the value is YYTABLE_NINF, it's a syntax error.
 
    YYPGOTO[I] = the index in YYTABLE of the portion describing what to
    do after reducing a rule that derives variable I + NTOKENS.  This
@@ -99,6 +99,16 @@
    YYCHECK[YYPGOTO[I] + S] != S), then the default state (that is,
    YYDEFGOTO[I]) should be used instead of YYTABLE.  Otherwise,
    YYTABLE[YYPGOTO[I] + S] is the state to go to even if YYPGOTO[I] < 0.
+
+   When the above YYPACT, YYPGOTO, and YYCHECK tests determine that a
+   value from YYTABLE should be used, that value is never zero, so it is
+   useless to check for zero.  When those tests indicate that the value
+   from YYDEFACT or YYDEFGOTO should be used instead, the value from
+   YYTABLE *might* be zero, which, as a consequence of the way in which
+   the tables are constructed, also happens to indicate that YYDEFACT or
+   YYDEFGOTO should be used.  However, the YYTABLE value cannot be
+   trusted when the YYDEFACT or YYDEFGOTO value should be used.  In
+   summary, forget about zero values in YYTABLE.
 */
 
 extern int nvectors;
-- 
1.5.4.3