Re: [PATCH 2/3] variants: minor improvements

[Théophile Ranquet]

2013/1/4 Akim Demaille <address@hidden>:
>
> Le 4 janv. 2013 à 13:55, Theophile Ranquet <address@hidden> a écrit :
>
>> * data/variant.hh (swap): More asserts can't hurt.
>> (variant): Prohibit copy construction.
>>
>> diff --git a/data/variant.hh b/data/variant.hh
>> index 1ebcfe8..afa825e 100644
>> --- a/data/variant.hh
>> +++ b/data/variant.hh
>> @@ -165,6 +165,8 @@ m4_define([b4_variant_define],
>>     inline void
>>     swap (variant<S>& other)
>>     {]b4_parse_assert_if([
>> +      YYASSERT (built);
>> +      YYASSERT (other.built);
>>       YYASSERT (tname == other.tname);])[
>>       std::swap (as<T>(), other.as<T>());]b4_parse_assert_if([
>>       std::swap (built, other.built);
>
> This seems fishy:

Agreed.

> if both are asserted to be true, why
> swapping them?  Maybe the function was intended to swap
> between constructed and not-constructed, if it does make
> sense.

Yes, this seems to have been the intention.  Note that the equality of
tnames is also asserted, (which seems a pretty sane thing to do).
There might have been some confusion with the following build
(variant<S>& other) function, which can be used to do exactly that
(swap constructed and non-constructed).

While the subject is assert-weirdness, I had to comment the asserts in
the variants's build(), because some skeletons build stuff on a
persistent variable, which means it is built more than once. This is
what will be installed. As the commit message says, I would like to
actually solve this some day.

commit c5d12324afe8ed3d113d46fb6f34fd9d0626a92c
Author: Theophile Ranquet <address@hidden>
Date:   Fri Dec 21 16:48:54 2012 +0100

    variants: assert changes

    * data/variant.hh (swap): More asserts can't hurt. Don't perform
useless swaps.
    (build): Deactivate problematic asserts, pending further investigation.
    (variant): Prohibit copy construction.

diff --git a/data/variant.hh b/data/variant.hh
index 1ebcfe8..184485c 100644
--- a/data/variant.hh
+++ b/data/variant.hh
@@ -106,8 +106,8 @@ m4_define([b4_variant_define],
     inline T&
     build ()
     {]b4_parse_assert_if([
-      YYASSERT (!built);
-      YYASSERT (!tname);
+      //YYASSERT (!built);
+      //YYASSERT (!tname);
       YYASSERT (sizeof (T) <= S);
       built = true;
       tname = typeid (T).name ();])[
@@ -119,8 +119,8 @@ m4_define([b4_variant_define],
     inline 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 ();])[
@@ -161,14 +161,15 @@ m4_define([b4_variant_define],
     }

     /// Swap the content with \a other, of same type.
+    /// Both variants must be built beforehand.
     template <typename T>
     inline void
     swap (variant<S>& other)
     {]b4_parse_assert_if([
+      YYASSERT (built);
+      YYASSERT (other.built);
       YYASSERT (tname == other.tname);])[
-      std::swap (as<T>(), other.as<T>());]b4_parse_assert_if([
-      std::swap (built, other.built);
-      std::swap (tname, other.tname);])[
+      std::swap (as<T>(), other.as<T>());
     }

     /// Assign the content of \a other to this.
@@ -208,6 +209,11 @@ m4_define([b4_variant_define],
       abort ();
     }

+    variant (const self_type&)
+    {
+      abort ();
+    }
+
   private:
     /// A buffer large enough to store any of the semantic values.
     /// Long double is chosen as it has the strongest alignment


I would also like to suggest the following commit, which gives a bit
of (much needed) clarification. The name "build' had the advantage
that it put emphasis on the preconditions, which are the same as those
that were intended for the 'build' functions: it must only be called
on a non-built variant. I added an assert to stress this point.

commit b3cdf838148ccc0d457878a7412a35bbdf1a01a2
Author: Theophile Ranquet <address@hidden>
Date:   Fri Jan 11 12:38:35 2013 +0100

    variants: document move and swap

    * data/variant.hh (swap): Doc.
    (build): Rename as...
    (move): This, more coherent naming with clearer meaning.
    * data/c++.m4 (move): Adjust.

diff --git a/data/c++.m4 b/data/c++.m4
index 4fb2805..855099f 100644
--- a/data/c++.m4
+++ b/data/c++.m4
@@ -306,7 +306,7 @@ m4_define([b4_public_types_define],
   {
     this->type = s.type_get ();]b4_locations_if([
     location = s.location;])[
-    ]b4_variant_if([b4_symbol_variant([s.type_get ()], [value], [build],
+    ]b4_variant_if([b4_symbol_variant([s.type_get ()], [value], [move],
                                       [s.value])],
                    [value = s.value;])[
   }
diff --git a/data/variant.hh b/data/variant.hh
index 4317aba..41286c5 100644
--- a/data/variant.hh
+++ b/data/variant.hh
@@ -161,7 +161,12 @@ m4_define([b4_variant_define],
     }

     /// Swap the content with \a other, of same type.
-    /// Both variants must be built beforehand.
+    /// Both variants must be built beforehand, because swapping the actual
+    /// data requires reading it (with as()), and this is not possible on
+    /// unconstructed variants: it would require some dynamic testing, which
+    /// should not be the variant's responsability.
+    /// Swapping between built and ((possibly) non-built is done with
+    /// variant::move ().
     template <typename T>
     inline void
     swap (variant<S>& other)
@@ -176,8 +181,9 @@ m4_define([b4_variant_define],
     /// Destroys \a other.
     template <typename T>
     inline void
-    build (variant<S>& other)
-    {
+    move (variant<S>& other)
+    {]b4_parse_assert_if([
+      YYASSERT (! built);])[
       build<T>();
       swap<T>(other);
       other.destroy<T>();