C++11 move semantics

[Frank Heckenbach]

Hi,

so far my C++ parsers use the C template (which works, but with
obvious restrictions). Now I'm trying to convert them to real C++
parsers using lalr1.cc.

One of the main advantages I hope to achieve is to use proper C++
classes for semantic values (where so far I've had to use manually
managed plain pointers). However, the generated parser requires
copyable types, just moveable isn't enough (even if I use std::move
in all user-defined actions).

Some of my semantic values are move-only types, some others are not
strictly non-copyable, but contain complex structures that I really
don't want to copy. unique_ptr obviously doesn't help. I could use
shared_ptr, but it doesn't have a proper[1] way to release objects
from shared_ptr, so I'd have to keep shared_ptr throughout the rest
of the code even if semantically there should be only one reference
to the object. This might cause noticeable performance problems (in
my code, the parser runs quickly, but the rest of the code is
somewhat time-critical and adding shared_ptr overhead all over the
place might be problematic).

[1] There are hacky workarounds, as discussed here:
    
https://stackoverflow.com/questions/1833356/detach-a-pointer-from-a-shared-ptr
    but I really don't want to get into that. Or one could use
    shared_ptr <unique_ptr <T>>, but that also gets ugly.

I can't imagine I'm the only one having this problem. What do other
C++11 users do in such cases?

I now see this seems to be an open problem for years. In
http://lists.gnu.org/archive/html/bug-bison/2015-01/msg00066.html
Hans Aberg made a promising approach. While it seems to remove all
copying at runtime, the code still contains calls to copy
constructors/assignment which are never reached in the test case,
but still prevent using move-only types.

So I went from there, made the changes in the templates rather than
the generated code, and hope to remove all copying of semantic types
(so I didn't even need a combined l- and r-value template with
std::forward, as was discussed back then).

Patch attached (against 3.0.4.dfsg-1+b1 from Debian stretch).

variant.hh:

- Basically the same changes Hans did, replacing const T& and copy
  by T&& and std::move where required.

- Optimized variant::move to use std::move instead of swap
  (swap would also still work, but often does too much).

- Disabled (#if 0) variant::copy to make sure it's never used.

- Explicitly disabled move constructor and assignment, otherwise the
  the templated constructor could be used instead accidentally
  (which would only fail at runtime with an assertion, as I found
  out while testing)!

- Changed static assertions from YYASSERT to static_assert
  (not strictly related, but useful if C++11 is used anyway).

c++.m4:

- basic_symbol: Constructor for symbols with semantic value:
  Use T&& and std::move, like above.

- Added move constructor/assignment for basic_symbol. Fortunately,
  I could reuse most of the code of its copy constructor, which must
  not exist anymore, to implement them.

lalr1.cc:

- Due to the previous change, stack_symbol_type::operator= becomes
  superfluous, so I took it out to make sure.

I'm still not totally happy with the code; in particular, it's now a
mixture of proper std::move usage, and existing functions taking an
lvalue reference to a steal from, but I didn't want to change much
more than necessary. And I only tested it with the calc++ example so
far (using "unique_ptr <string>"), so there may be problems hidden,
and the non-variant case is not tested at all.

I'd appreciate any feedback from the maintainers or other users who
have the same problem.

Regards,
Frank

--- orig/variant.hh     2015-08-05 09:05:30.000000000 +0200
+++ ./variant.hh        2018-03-03 21:17:13.880113143 +0100
@@ -100,11 +100,11 @@
 
     /// Construct and fill.
     template <typename T>
-    variant (const T& t)]b4_parse_assert_if([
+    variant (T&& t)]b4_parse_assert_if([
       : yytypeid_ (&typeid (T))])[
     {
-      YYASSERT (sizeof (T) <= S);
-      new (yyas_<T> ()) T (t);
+      static_assert (sizeof (T) <= S, "variant size too small");
+      new (yyas_<T> ()) T (std::move (t));
     }
 
     /// Destruction, allowed only if empty.
@@ -119,7 +119,7 @@
     build ()
     {]b4_parse_assert_if([
       YYASSERT (!yytypeid_);
-      YYASSERT (sizeof (T) <= S);
+      static_assert (sizeof (T) <= S, "variant size too small");
       yytypeid_ = & typeid (T);])[
       return *new (yyas_<T> ()) T;
     }
@@ -127,12 +127,12 @@
     /// Instantiate a \a T in here from \a t.
     template <typename T>
     T&
-    build (const T& t)
+    build (T &&t)
     {]b4_parse_assert_if([
       YYASSERT (!yytypeid_);
-      YYASSERT (sizeof (T) <= S);
+      static_assert (sizeof (T) <= S, "variant size too small");
       yytypeid_ = & typeid (T);])[
-      return *new (yyas_<T> ()) T (t);
+      return *new (yyas_<T> ()) T (std::move (t));
     }
 
     /// Accessor to a built \a T.
@@ -141,7 +141,7 @@
     as ()
     {]b4_parse_assert_if([
       YYASSERT (*yytypeid_ == typeid (T));
-      YYASSERT (sizeof (T) <= S);])[
+      static_assert (sizeof (T) <= S, "variant size too small");])[
       return *yyas_<T> ();
     }
 
@@ -151,7 +151,7 @@
     as () const
     {]b4_parse_assert_if([
       YYASSERT (*yytypeid_ == typeid (T));
-      YYASSERT (sizeof (T) <= S);])[
+      static_assert (sizeof (T) <= S, "variant size too small");])[
       return *yyas_<T> ();
     }
 
@@ -179,11 +179,14 @@
     void
     move (self_type& other)
     {
-      build<T> ();
-      swap<T> (other);
+      build<T> ();]b4_parse_assert_if([
+      YYASSERT (yytypeid_);
+      YYASSERT (*yytypeid_ == *other.yytypeid_);])[
+      as<T> () = std::move (other.as<T> ());
       other.destroy<T> ();
     }
 
+#if 0
     /// Copy the content of \a other to this.
     template <typename T>
     void
@@ -191,6 +194,7 @@
     {
       build<T> (other.as<T> ());
     }
+#endif
 
     /// Destroy the stored \a T.
     template <typename T>
@@ -202,9 +206,12 @@
     }
 
   private:
-    /// Prohibit blind copies.
+    /// Prohibit blind copies
+    /// Don't use the templated constructor (which would only fail at runtime 
with an assertion)!
     self_type& operator=(const self_type&);
+    self_type& operator=(self_type&&);
     variant (const self_type&);
+    variant (self_type&&);
 
     /// Accessor to raw memory as \a T.
     template <typename T>
@@ -290,7 +297,7 @@
     symbol_type
     make_[]b4_symbol_([$1], [id]) (dnl
 b4_join(b4_symbol_if([$1], [has_type],
-                     [const b4_symbol([$1], [type])& v]),
+                     [b4_symbol([$1], [type])&& v]),
         b4_locations_if([const location_type& l])));
 
 ])])])
@@ -314,11 +321,11 @@
 [  b4_parser_class_name::symbol_type
   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_symbol([$1], [type])&& v]),
         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_symbol_if([$1], [has_type], [std::move 
(v)]),
                                 b4_locations_if([l])));
   }
 
@@ -332,7 +339,7 @@
 [[
   basic_symbol (]b4_join(
           [typename Base::kind_type t],
-          b4_symbol_if([$1], [has_type], const b4_symbol([$1], [type])[ v]),
+          b4_symbol_if([$1], [has_type], b4_symbol([$1], [type])&&[ v]),
           b4_locations_if([const location_type& l]))[);
 ]])
 
@@ -344,10 +351,10 @@
   template <typename Base>
   ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (]b4_join(
           [typename Base::kind_type t],
-          b4_symbol_if([$1], [has_type], const b4_symbol([$1], [type])[ v]),
+          b4_symbol_if([$1], [has_type], b4_symbol([$1], [type])&&[ v]),
           b4_locations_if([const location_type& l]))[)
     : Base (t)
-    , value (]b4_symbol_if([$1], [has_type], [v])[)]b4_locations_if([
+    , value (]b4_symbol_if([$1], [has_type], [std::move 
(v)])[)]b4_locations_if([
     , location (l)])[
   {}
 ]])
--- orig/c++.m4 2015-08-05 09:05:30.000000000 +0200
+++ ./c++.m4    2018-03-03 21:08:36.253716197 +0100
@@ -198,8 +198,9 @@
       /// Default constructor.
       basic_symbol ();
 
-      /// Copy constructor.
-      basic_symbol (const basic_symbol& other);
+      /// Move constructor and assignment.
+      basic_symbol (basic_symbol&& other);
+      basic_symbol& operator= (basic_symbol&& other);
 ]b4_variant_if([[
       /// Constructor for valueless symbols, and symbols from each type.
 ]b4_type_foreach([b4_basic_symbol_constructor_declare])], [[
@@ -209,7 +210,7 @@
 
       /// Constructor for symbols with semantic value.
       basic_symbol (typename Base::kind_type t,
-                    const semantic_type& v]b4_locations_if([,
+                    semantic_type&& v]b4_locations_if([,
                     const location_type& l])[);
 
       /// Destroy the symbol.
@@ -231,7 +232,8 @@
       location_type location;])[
 
     private:
-      /// Assignment operator.
+      /// This class is not copyable.
+      basic_symbol (const basic_symbol& other);
       basic_symbol& operator= (const basic_symbol& other);
     };
 
@@ -294,29 +296,35 @@
 
   template <typename Base>
   inline
-  ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (const 
basic_symbol& other)
-    : Base (other)
-    , value ()]b4_locations_if([
-    , location (other.location)])[
+  ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (basic_symbol&& 
other)
   {
-    ]b4_variant_if([b4_symbol_variant([other.type_get ()], [value], [copy],
-                                      [other.value])],
-                   [value = other.value;])[
+    *this = std::move (other);
   }
 
+  template <typename Base>
+  inline
+  ]b4_parser_class_name[::basic_symbol<Base>& 
]b4_parser_class_name[::basic_symbol<Base>::operator= (basic_symbol&& other)
+  {
+    static_cast<Base &> (*this) = other;]b4_locations_if([
+    location = std::move (other.location);])[
+    ]b4_variant_if([b4_symbol_variant([other.type_get ()], [value], [move],
+                                      [other.value])],
+                   [value = std::move (other.value);])[
+    return *this;
+  }
 
   template <typename Base>
   inline
   ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (]b4_join(
           [typename Base::kind_type t],
-          [const semantic_type& v],
+          [semantic_type&& v],
           b4_locations_if([const location_type& l]))[)
     : Base (t)
-    , value (]b4_variant_if([], [v])[)]b4_locations_if([
+    , value (]b4_variant_if([], [std::move (v)])[)]b4_locations_if([
     , location (l)])[
   {]b4_variant_if([[
     (void) v;
-    ]b4_symbol_variant([this->type_get ()], [value], [copy], [v])])[}
+    ]b4_symbol_variant([this->type_get ()], [value], [move], [v])])[}
 
 ]b4_variant_if([[
   // Implementation of basic_symbol constructor for each type.
--- orig/lalr1.cc       2015-08-05 09:05:30.000000000 +0200
+++ ./lalr1.cc  2018-03-03 20:54:45.326675277 +0100
@@ -316,8 +316,6 @@
       stack_symbol_type ();
       /// Steal the contents from \a sym to build this.
       stack_symbol_type (state_type s, symbol_type& sym);
-      /// Assignment, needed by push_back.
-      stack_symbol_type& operator= (const stack_symbol_type& that);
     };
 
     /// Stack type.
@@ -592,6 +590,7 @@
     that.type = empty_symbol;
   }
 
+#if 0
   inline
   ]b4_parser_class_name[::stack_symbol_type&
   ]b4_parser_class_name[::stack_symbol_type::operator= (const 
stack_symbol_type& that)
@@ -603,6 +602,7 @@
     location = that.location;])[
     return *this;
   }
+#endif
 
 
   template <typename Base>