Re: Multiple defs in union

[Akim Demaille]

Hi Rich,

> Le 20 déc. 2014 à 00:08, Rich Wilson <address@hidden> a écrit :
> 
> The following syntax file parse.y:
> =====================================
> %defines
> 
> %define api.value.type union
> 
> %token <int> NUM
> 
> %%
> 
> input: NUM
>     { printf("%d\n", $1); }
>    ;
> 
> %%
> ====================================
> causes NUM to be defined multiple times.
> Excerpt from parse.tab.h:
> ====================================
> union YYSTYPE
> {
> 
> #line 59 "parse.tab.h" /* yacc.c:1909  */
>  /* NUM  */
>  int NUM;
>  /* NUM  */
>  int NUM;
> };

Wow.  This is really ugly, thanks a lot for the report.
I will release Bison 3.0.3 as soon as possible.  Too bad
the test suite did not check the use of %defines, it could
have caught it much earlier.

I'm installing the following changes.

commit 8d4dc896cb3aac3afaf82130a137c9d1afe37e48
Author: Akim Demaille <address@hidden>
Date:   Wed Dec 31 14:15:06 2014 +0100

    yacc.c: fix broken union when api.value.type=union and %defines are used
    
    Reported by Rich Wilson.
    
    * data/c.m4 (b4_symbol_type_register): Append to b4_union_members,
    not b4_user_union_members.
    The latter invokes the former, but it is the former which is reinitialized
    to empty by b4_value_type_setup_union.
    * tests/types.at: Check it.
    
    This reveals another bug, this time in the case of glr.c parsers.
    
    * data/glr.c: Generate the header file before the implementation file,
    to be sure that the setup is run before what depends on it.

diff --git a/NEWS b/NEWS
index 9c5cb42..cb2cd64 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,11 @@ GNU Bison NEWS
 
 ** Bug fixes
 
+*** %define api.value.type union with %defines
+
+  The yacc.c and glr.c parsers were broken when %defines was used
+  together with "%define api.value.type union".
+
 *** Redeclarations are reported in proper order
 
   On
diff --git a/THANKS b/THANKS
index 780ecd5..1b1ff71 100644
--- a/THANKS
+++ b/THANKS
@@ -112,6 +112,7 @@ Quoc Peyrot               address@hidden
 R Blake                   address@hidden
 Raja R Harinath           address@hidden
 Ralf Wildenhues           address@hidden
+Rich Wilson               address@hidden
 Richard Stallman          address@hidden
 Rici Lake                 address@hidden
 Rob Conde                 address@hidden
diff --git a/data/c.m4 b/data/c.m4
index 7f85144..1f1a23f 100644
--- a/data/c.m4
+++ b/data/c.m4
@@ -560,15 +560,15 @@ b4_locations_if([, yylocationp])[]b4_user_args[);
 # b4_symbol_type_register(SYMBOL-NUM)
 # -----------------------------------
 # Symbol SYMBOL-NUM has a type (for variant) instead of a type-tag.
-# Extend the definition of %union's body with a field of that type,
-# and extend the symbol's "type" field to point to the field name,
-# instead of the type name.
+# Extend the definition of %union's body (b4_union_members) with a
+# field of that type, and extend the symbol's "type" field to point to
+# the field name, instead of the type name.
 m4_define([b4_symbol_type_register],
 [m4_define([b4_symbol($1, type_tag)],
            [b4_symbol_if([$1], [has_id],
                          [b4_symbol([$1], [id])],
                          [yytype_[]b4_symbol([$1], [number])])])dnl
-m4_append([b4_user_union_members],
+m4_append([b4_union_members],
 m4_expand([
   b4_symbol_tag_comment([$1])dnl
   b4_symbol([$1], [type]) b4_symbol([$1], [type_tag]);]))
diff --git a/data/glr.c b/data/glr.c
index 236a4b4..f34af1f 100644
--- a/data/glr.c
+++ b/data/glr.c
@@ -178,6 +178,39 @@ m4_if(b4_skeleton, ["glr.c"],
 ## Output files.  ##
 ## -------------- ##
 
+# Unfortunately the order of generation between the header and the
+# implementation file matters (for glr.c) because of the current
+# implementation of api.value.type=union.  In that case we still use a
+# union for YYSTYPE, but we generate the contents of this union when
+# setting up YYSTYPE.  This is needed for other aspects, such as
+# defining yy_symbol_value_print, since we need to now the name of the
+# members of this union.
+#
+# To avoid this issue, just generate the header before the
+# implementation file.  But we should also make them more independant.
+
+# ----------------- #
+# The header file.  #
+# ----------------- #
+
+# glr.cc produces its own header.
+m4_if(b4_skeleton, ["glr.c"],
+[b4_defines_if(
+[b4_output_begin([b4_spec_defines_file])
+b4_copyright([Skeleton interface for Bison GLR parsers in C],
+             [2002-2014])[
+
+]b4_cpp_guard_open([b4_spec_defines_file])[
+]b4_shared_declarations[
+]b4_cpp_guard_close([b4_spec_defines_file])[
+]b4_output_end()
+])])
+
+
+# ------------------------- #
+# The implementation file.  #
+# ------------------------- #
+
 b4_output_begin([b4_parser_file_name])
 b4_copyright([Skeleton implementation for Bison GLR parsers in C],
              [2002-2014])[
@@ -2550,16 +2583,3 @@ m4_if(b4_prefix, [yy], [],
 
 ]b4_epilogue[]dnl
 b4_output_end()
-
-# glr.cc produces its own header.
-m4_if(b4_skeleton, ["glr.c"],
-[b4_defines_if(
-[b4_output_begin([b4_spec_defines_file])
-b4_copyright([Skeleton interface for Bison GLR parsers in C],
-             [2002-2014])[
-
-]b4_cpp_guard_open([b4_spec_defines_file])[
-]b4_shared_declarations[
-]b4_cpp_guard_close([b4_spec_defines_file])[
-]b4_output_end()
-])])
diff --git a/tests/types.at b/tests/types.at
index 2c9203e..29f51e2 100644
--- a/tests/types.at
+++ b/tests/types.at
@@ -61,15 +61,16 @@ AT_CLEANUP
 ## api.value.type.  ##
 ## ---------------- ##
 
-# AT_TEST($1: BISON-DIRECTIVES,
-#         $2: MORE-BISON-DIRECTIVES,
-#         $3: PARSER-ACTION,
-#         $4: INPUT, $5: SCANNER-ACTION,
-#         $6: RESULT)
+# _AT_TEST($1: BISON-DIRECTIVES,
+#          $2: MORE-BISON-DIRECTIVES,
+#          $3: PARSER-ACTION,
+#          $4: INPUT,
+#          $5: SCANNER-ACTION,
+#          $6: RESULT)
 # --------------------------------------
 # Compile the grammar and check the expected result.
 # BISON-DIRECTIVES are passed to AT_SETUP, contrary to MORE-BISON-DIRECTIVES.
-m4_pushdef([AT_TEST],
+m4_pushdef([_AT_TEST],
 [
 AT_SETUP([$1])
 AT_KEYWORDS([api.value.type])
@@ -105,6 +106,25 @@ AT_BISON_OPTION_POPDEFS
 AT_CLEANUP
 ])
 
+# AT_TEST($1: BISON-DIRECTIVES,
+#         $2: MORE-BISON-DIRECTIVES,
+#         $3: PARSER-ACTION,
+#         $4: INPUT,
+#         $5: SCANNER-ACTION,
+#         $6: RESULT)
+# --------------------------------------
+# Check with and without %defines, to avoid regressions.  It turns out
+# that in that case yacc.c calls the set-up of the %union twice,
+# because YYSTYPE is defined once in the header, and once in the
+# implementation file (eventually it'd be better to include the header
+# file, but that's another story).  Unfortunately running these macros
+# a second time doubled the side-effects and resulted in a double
+# definition of the union members.
+m4_pushdef([AT_TEST],
+[_AT_TEST([$1],          [$2], [$3], [$4], [$5], [$6])
+ _AT_TEST([$1 %defines], [$2], [$3], [$4], [$5], [$6])
+])
+
 m4_foreach([b4_skel], [[yacc.c], [glr.c], [lalr1.cc], [glr.cc]],
  [# A built-in type.
   AT_TEST([%skeleton "]b4_skel["
@@ -228,3 +248,4 @@ m4_foreach([b4_skel], [[yacc.c], [glr.c], [lalr1.cc], 
[glr.cc]],
 ])
 
 m4_popdef([AT_TEST])
+m4_popdef([_AT_TEST])