[PATCH 2/6] c++: revamp the support for variants

[Akim Demaille]

The current approach was too adhoc: the symbols were not sufficiently
self-contained, in particular wrt memory management.  The "new"
guideline is the one that should have been followed from the start:
let the symbols handle themslves, instead of leaving their users to
it.  It was justified by the will to avoid gratuitious moves and
copies, but the current approach does not seem to be slower, yet it
will probably be simpler to adjust to support move semantics from
C++11.

The documentation says that the %parse-param are available from the
%destructor.  In retrospect, that was a silly design decision, which
we can break for variants, as its a new feature.  It should be phased
out for non-variants too.

* data/variant.hh: A variant never knows if it stores something or
not, it is up to its users to store this information.
Yet, in parse.assert mode, make sure the empty/filled variants
are properly used.
(b4_symbol_constructor_define_): Don't call directly the symbol
constructor, to save a useless temporary.
* data/stack.hh (push): Steal the pushed value instead of duplicating
it.
This will simplify the callers of push, who handled this "move"
approach themselves.
* data/c++.m4 (basic_symbol): Let -1, as kind, denote the fact that
a symbol is empty.
This is needed for instance when shifting the lookahead: yyla
is given as argument to "push", and its value is then moved on
the stack.  But then yyla must be declared "empty" so that its
destructor won't be called.
(basic_symbol::move): New.
Move the responsibility of calling the destructor from yy_destroy
to ~basic_symbol in the case of variants.
* data/lalr1.cc (stack_symbol_type): Now a derived class from its
previous value, so that we can add a constructor from a symbol_type.
(by_state): State -1 means empty.
(yypush_): Factor, by calling one overload from the other one, and
using the new semantics of stack::push.
No longer reclaim by hand the memory from rhs symbols, since now
that we store objects with proper destructors, they will be reclaimed
automatically.
Conversely, be sure to delete yylhs.
* tests/c++.at (C++ Variant-based Symbols): New "unit" test for
symbols.
---
 data/c++.m4     | 60 +++++++++++++++++++++++++++---------
 data/lalr1.cc   | 96 +++++++++++++++++++++++++++++++--------------------------
 data/stack.hh   |  8 +++--
 data/variant.hh | 47 ++++++++++++++++------------
 tests/c++.at    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 217 insertions(+), 79 deletions(-)

diff --git a/data/c++.m4 b/data/c++.m4
index f9fcb9d..591e3e8 100644
--- a/data/c++.m4
+++ b/data/c++.m4
@@ -166,6 +166,9 @@ m4_define([b4_public_types_declare],
     template <typename Base>
     struct basic_symbol : Base
     {
+      /// Alias to Base.
+      typedef Base super_type;
+
       /// Default constructor.
       inline basic_symbol ();
 ]b4_locations_if([
@@ -184,6 +187,7 @@ m4_define([b4_public_types_declare],
                            const semantic_type& v]b4_locations_if([,
                            const location_type& l])[);
 
+      ~basic_symbol ();
       /// Assignment operator.
       inline basic_symbol& operator= (const basic_symbol& other);
 
@@ -209,10 +213,17 @@ m4_define([b4_public_types_declare],
       /// Constructor.
       inline by_type (token_type t);
 
+      /// Steal the type of \a that.
+      void move (by_type& that);
+
       /// The symbol type.
+      ///
+      /// -1 when this symbol is empty.
       int type;
 
       /// The type (corresponding to \a type).
+      ///
+      /// -1 when this symbol is empty.
       inline int type_get () const;
 
       /// The token.
@@ -275,39 +286,52 @@ m4_define([b4_public_types_define],
           [const semantic_type& v],
           b4_locations_if([const location_type& l]))[)
     : Base (t)
-    , value ()]b4_locations_if([
+    , value (]b4_variant_if([], [v])[)]b4_locations_if([
     , location (l)])[
-  {
-    // FIXME: The YYUSE macro is only available in the .cc skeleton files.  It
-    // is not available in .hh files, where this code is when using %defines.
+  {]b4_variant_if([[
     (void) v;
-    ]b4_variant_if([b4_symbol_variant([this->type_get ()], [value], [copy],
-                                      [v])],
-                   [value = v;])[
-  }
+    ]b4_symbol_variant([this->type_get ()], [value], [copy], [v])])[}
 
   template <typename Base>
   ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (]b4_join(
           [typename Base::value_type t],
           b4_locations_if([const location_type& l]))[)
-    : Base (t)]b4_locations_if([
+    : Base (t)
+    , value ()]b4_locations_if([
     , location (l)])[
   {}
 
   template <typename Base>
+  inline
+  ]b4_parser_class_name[::basic_symbol<Base>::~basic_symbol ()
+  {]b4_variant_if([[
+    // User destructor.
+    int yytype = this->type_get ();
+    switch (yytype)
+    {
+]b4_symbol_foreach([b4_symbol_destructor])dnl
+[   default:
+      break;
+    }
+
+    // Type destructor.
+  ]b4_symbol_variant([[yytype]], [[value]], [[template destroy]])])[
+  }
+
+  template <typename Base>
   void
   ]b4_parser_class_name[::basic_symbol<Base>::move (basic_symbol& s)
   {
-    this->type = s.type_get ();]b4_locations_if([
-    location = s.location;])[
-    ]b4_variant_if([b4_symbol_variant([s.type_get ()], [value], [move],
+    super_type::move(s);
+    ]b4_variant_if([b4_symbol_variant([this->type_get ()], [value], [move],
                                       [s.value])],
-                   [value = s.value;])[
+                   [value = s.value;])[]b4_locations_if([
+    location = s.location;])[
   }
 
   // by_type.
   ]b4_parser_class_name[::by_type::by_type ()
-     : type ()
+     : type (-1)
   {}
 
   ]b4_parser_class_name[::by_type::by_type (const by_type& other)
@@ -318,6 +342,14 @@ m4_define([b4_public_types_define],
     : type (yytranslate_ (t))
   {}
 
+  inline
+  void
+  ]b4_parser_class_name[::by_type::move (by_type& that)
+  {
+    type = that.type;
+    that.type = -1;
+  }
+
   int
   ]b4_parser_class_name[::by_type::type_get () const
   {
diff --git a/data/lalr1.cc b/data/lalr1.cc
index a195602..5a05db7 100644
--- a/data/lalr1.cc
+++ b/data/lalr1.cc
@@ -281,10 +281,18 @@ b4_location_define])])[
       /// Copy constructor.
       inline by_state (const by_state& other);
 
+      void move (by_state& that)
+      {
+        state = that.state;
+        that.state = -1;
+      }
+
       /// The state.
       state_type state;
 
       /// The type (corresponding to \a state).
+      ///
+      /// -1 when empty.
       inline int type_get () const;
 
       /// The type used to store the symbol type.
@@ -292,7 +300,16 @@ b4_location_define])])[
     };
 
     /// "Internal" symbol: element of the stack.
-    typedef basic_symbol<by_state> stack_symbol_type;
+    struct stack_symbol_type : basic_symbol<by_state>
+    {
+      /// Superclass.
+      typedef basic_symbol<by_state> super_type;
+      /// Construct an empty symbol.
+      stack_symbol_type ();
+      /// Steal the contents from \a sym to build this.
+      stack_symbol_type (state_type s, symbol_type& sym);
+      stack_symbol_type& operator= (const stack_symbol_type& that);
+    };
 
     /// Stack type.
     typedef stack<stack_symbol_type> stack_type;
@@ -512,7 +529,7 @@ m4_if(b4_prefix, [yy], [],
 
   // by_state.
   ]b4_parser_class_name[::by_state::by_state ()
-    : state ()
+    : state (-1)
   {}
 
   ]b4_parser_class_name[::by_state::by_state (const by_state& other)
@@ -526,7 +543,33 @@ m4_if(b4_prefix, [yy], [],
   int
   ]b4_parser_class_name[::by_state::type_get () const
   {
-    return yystos_[state];
+    return state == -1 ? -1 : yystos_[state];
+  }
+
+  inline
+  ]b4_parser_class_name[::stack_symbol_type::stack_symbol_type ()
+  {}
+
+
+  inline
+  ]b4_parser_class_name[::stack_symbol_type::stack_symbol_type (state_type s, 
symbol_type& sym)
+    : super_type (s]b4_locations_if([, sym.location])[)
+  {]b4_variant_if([[
+    ]b4_symbol_variant([sym.type_get ()], [value], [move], [sym.value])],
+    [value = sym.value;])[
+    // sym is emptied.
+    sym.type = -1;
+  }
+
+  inline
+  ]b4_parser_class_name[::stack_symbol_type&
+  ]b4_parser_class_name[::stack_symbol_type::operator= (const 
stack_symbol_type& that)
+  {
+    state = that.state;]b4_variant_if([[
+    ]b4_symbol_variant([that.type_get ()], [value], [copy], [that.value])],
+    [value = that.value;])[]b4_locations_if([
+    location = that.location;])[
+    return *this;
   }
 
 
@@ -535,7 +578,7 @@ m4_if(b4_prefix, [yy], [],
   ]b4_parser_class_name[::yy_destroy_ (const char* yymsg, basic_symbol<Base>& 
yysym) const
   {
     if (yymsg)
-      YY_SYMBOL_PRINT (yymsg, yysym);
+      YY_SYMBOL_PRINT (yymsg, yysym);]b4_variant_if([], [
 
     // User destructor.
     int yytype = yysym.type_get ();
@@ -544,10 +587,7 @@ m4_if(b4_prefix, [yy], [],
 ]b4_symbol_foreach([b4_symbol_destructor])dnl
 [       default:
           break;
-      }]b4_variant_if([
-
-    // Type destructor.
-  b4_symbol_variant([[yytype]], [[yysym.value]], [[template destroy]])])[
+      }])[
   }
 
 #if ]b4_api_PREFIX[DEBUG
@@ -575,17 +615,8 @@ m4_if(b4_prefix, [yy], [],
   void
   ]b4_parser_class_name[::yypush_ (const char* m, state_type s, symbol_type& 
sym)
   {
-    if (m)
-      YY_SYMBOL_PRINT (m, sym);
-]b4_variant_if(
-[[
-  stack_symbol_type ss (]b4_join([s],
-      [sym.value], b4_locations_if([sym.location]))[);
-  ]b4_symbol_variant([sym.type_get ()], [sym.value], [destroy], [])[;
-  yystack_.push (ss);
-]],
-[[    yystack_.push (stack_symbol_type (]b4_join([s],
-                         [sym.value], b4_locations_if([sym.location]))[));]])[
+    stack_symbol_type t (s, sym);
+    yypush_ (m, t);
   }
 
   void
@@ -593,14 +624,7 @@ m4_if(b4_prefix, [yy], [],
   {
     if (m)
       YY_SYMBOL_PRINT (m, s);
-]b4_variant_if(
-[[
-  stack_symbol_type ss (]b4_join([s.state],
-      [s.value], b4_locations_if([s.location]))[);
-  ]b4_symbol_variant([s.type_get ()], [s.value], [destroy], [])[;
-  yystack_.push (ss);
-]],
-[    yystack_.push (s);])[
+    yystack_.push (s);
   }
 
   void
@@ -821,21 +845,6 @@ b4_dollar_popdef])[]dnl
         YYERROR;
       }
     YY_SYMBOL_PRINT ("-> $$ =", yylhs);
-]b4_variant_if([[
-    // Destroy the rhs symbols.
-    for (int i = 0; i < yylen; ++i)
-      // Destroy a variant whose value may have been swapped with
-      // yylhs.value (for instance if the action was "std::swap($$,
-      // $1)").  The value of yylhs.value (hence possibly one of these
-      // rhs symbols) depends on the default construction for this
-      // type.  In the case of pointers for instance, no
-      // initialization is done, so the value is junk.  Therefore do
-      // not try to report the value of symbols about to be destroyed
-      // in the debug trace, it's possibly junk.  Hence yymsg = 0.
-      // Besides, that keeps exactly the same traces as with the other
-      // Bison skeletons.
-      yy_destroy_ (YY_NULL, yystack_[i]);]])[
-
     yypop_ (yylen);
     yylen = 0;
     YY_STACK_PRINT ();
@@ -890,7 +899,8 @@ b4_dollar_popdef])[]dnl
       goto yyerrorlab;]b4_locations_if([[
     yyerror_range[1].location = yystack_[yylen - 
1].location;]])b4_variant_if([[
     /* $$ was initialized before running the user action.  */
-    yy_destroy_ ("Error: discarding", yylhs);]])[
+    YY_SYMBOL_PRINT ("Error: discarding", yylhs);
+    yylhs.~stack_symbol_type();]])[
     /* Do not reclaim the symbols of the rule whose action triggered
        this YYERROR.  */
     yypop_ (yylen);
diff --git a/data/stack.hh b/data/stack.hh
index 7ed5560..037c212 100644
--- a/data/stack.hh
+++ b/data/stack.hh
@@ -53,11 +53,15 @@ m4_define([b4_stack_define],
       return seq_[seq_.size () - 1 - i];
     }
 
+    /// Steal the contents of \a t.
+    ///
+    /// Close to move-semantics.
     inline
     void
-    push (const T& t)
+    push (T& t)
     {
-      seq_.push_back (t);
+      seq_.push_back (T());
+      operator[](0).move (t);
     }
 
     inline
diff --git a/data/variant.hh b/data/variant.hh
index c96cfeb..bbae8cb 100644
--- a/data/variant.hh
+++ b/data/variant.hh
@@ -100,13 +100,29 @@ m4_define([b4_variant_define],
       , tname (YY_NULL)])[
     {}
 
-    /// Instantiate a \a T in here.
+    /// Construct and fill.
+    template <typename T>
+    variant (const T& t)]b4_parse_assert_if([
+      : built (true)
+      , tname (typeid (T).name ())])[
+    {
+      YYASSERT (sizeof (T) <= S);
+      new (buffer.raw) T (t);
+    }
+
+    /// Destruction, allowed only if empty.
+    ~variant ()
+    {]b4_parse_assert_if([
+      YYASSERT (!built);
+    ])[}
+
+    /// Instantiate an empty \a T in here.
     template <typename T>
     T&
     build ()
     {]b4_parse_assert_if([
-      //YYASSERT (!built);
-      //YYASSERT (!tname);
+      YYASSERT (!built);
+      YYASSERT (!tname);
       YYASSERT (sizeof (T) <= S);
       built = true;
       tname = typeid (T).name ();])[
@@ -118,24 +134,14 @@ m4_define([b4_variant_define],
     T&
     build (const T& t)
     {]b4_parse_assert_if([
-      //YYASSERT (!built);
-      //YYASSERT (!tname);
+      YYASSERT (!built);
+      YYASSERT (!tname);
       YYASSERT (sizeof (T) <= S);
       built = true;
       tname = typeid (T).name ();])[
       return *new (buffer.raw) T (t);
     }
 
-    /// Construct and fill.
-    template <typename T>
-    variant (const T& t)]b4_parse_assert_if([
-      : built (true)
-      , tname (typeid (T).name ())])[
-    {
-      YYASSERT (sizeof (T) <= S);
-      new (buffer.raw) T (t);
-    }
-
     /// Accessor to a built \a T.
     template <typename T>
     T&
@@ -314,14 +320,15 @@ m4_define([b4_symbol_constructor_define_],
   b4_parser_class_name::make_[]b4_symbol_([$1], [id]) (dnl
 b4_join(b4_symbol_if([$1], [has_type],
                      [const b4_symbol([$1], [type])& v]),
-        b4_locations_if([const location_type& l])))
+        b4_locations_if([const location_type& l])))[
   {
-    return symbol_type (b4_join([token::b4_symbol([$1], [id])],
-                                b4_symbol_if([$1], [has_type], [v]),
-                                b4_locations_if([l])));
+    symbol_type res (token::]b4_symbol([$1], [id])[]b4_locations_if([, l])[);
+    ]b4_symbol_if([$1], [has_type], [res.value.build (v);])[
+    //    ]b4_locations_if([res.location = l;])[
+    return res;
   }
 
-])])])
+]])])])
 
 
 # b4_symbol_constructor_define
diff --git a/tests/c++.at b/tests/c++.at
index e5cd069..9b52900 100644
--- a/tests/c++.at
+++ b/tests/c++.at
@@ -18,6 +18,91 @@
 AT_BANNER([[C++ Features.]])
 
 
+## --------------------------- ##
+## C++ Variant-based Symbols.  ##
+## --------------------------- ##
+
+AT_SETUP([C++ Variant-based Symbols])
+
+AT_KEYWORDS([variant])
+
+AT_BISON_OPTION_PUSHDEFS([%skeleton "lalr1.cc" %debug $1])
+# Store strings and integers in a list of strings.
+AT_DATA_GRAMMAR([list.yy],
+[[%skeleton "lalr1.cc"
+%define api.value.type variant
+%define parse.assert
+%debug
+
+%code top
+{
+  // Get access to stack_symbol_type for the tests.
+# define private public
+}
+%code provides
+{
+  ]AT_YYLEX_DECLARE[
+}
+
+%token <int> INT "int"
+%type < std::list<int> > exp
+
+%printer { yyo << $$; } <int>
+%printer
+  {
+    for (std::list<int>::const_iterator i = $$.begin (); i != $$.end (); ++i)
+      {
+        if (i != $$.begin ())
+          yyo << ", ";
+        yyo << *i;
+      }
+  } < std::list<int> >
+
+%code requires { #include <list> }
+%code { int yylex (yy::parser::semantic_type* yylval); }
+
+%%
+exp: "int" { $$.push_back ($1); }
+%%
+]AT_YYERROR_DEFINE[
+]AT_YYLEX_DEFINE[
+
+int main()
+{
+  {
+    yy::parser::symbol_type s = yy::parser::make_INT(12);
+    std::cerr << s.value.as<int>() << std::endl;
+  }
+
+  {
+    yy::parser::symbol_type s = yy::parser::make_INT(123);
+    yy::parser::stack_symbol_type ss(1, s);
+    std::cerr << ss.value.as<int>() << std::endl;
+  }
+
+  {
+    yy::parser::stack_type st;
+    for (int i = 0; i < 100; ++i)
+      {
+        yy::parser::symbol_type s(yy::parser::make_INT(i));
+        yy::parser::stack_symbol_type ss(1, s);
+        st.push(ss);
+      }
+  }
+}
+]])
+
+AT_BISON_CHECK([-o list.cc list.yy])
+AT_COMPILE_CXX([list], [$NO_STRICT_ALIAS_CXXFLAGS list.cc])
+AT_PARSER_CHECK([./list], 0, [],
+[12
+123
+])
+
+AT_BISON_OPTION_POPDEFS
+AT_CLEANUP
+
+
 ## ---------- ##
 ## Variants.  ##
 ## ---------- ##
-- 
1.8.1.1