commit dfee6a7b9491fd9b5e4d570d9dbc0336fa2e9acb
author Corey Tabaka <eieio@google.com> Thu Jun 01 00:24:49 2017 -0700
committer Corey Tabaka <eieio@google.com> Fri Jun 02 11:24:14 2017 -0700
tree 75bc6fb3c11bc2aa1c80b720e13eda2c3ffbb867
parent 5d5c530a8b9fa641547fdf68b4c577d73685011c

libpdx: Fix bug in Variant type.

Fix a subtle bug in the Variant value destruction along the
EmptyVariant assignment path:

    some_variant = EmptyVariant{};

The problem arises from the private utility method Destruct(),
which does not set the type index to "empty" after destroying the
current sub-element. For most paths this is okay because the type
index is immediately set to the new sub-element type. However, the
EmptyVariant path does not assign a new type because the Variant
should become empty. Since the type is not set to "empty" the
Variant incorrectly double destroys the previous value when a new
value is assigned.

Also clean up a superfluous overload of Assign() that is skipped
due to the stronger binding of the universal reference overload
Assign(T&&).

Update tests to properly cover this case. In the process discovered
two incorrect tests related to this issue and updated them.

Bug: None
Test: pdx_tests passes.
Change-Id: I106f863b34f2719820d04d0e701968332f659c3e

libs/vr/libpdx/private/pdx/rpc/variant.h

diff --git a/libs/vr/libpdx/private/pdx/rpc/variant.h b/libs/vr/libpdx/private/pdx/rpc/variant.h
index cb44a51..dc321fa 100644
--- a/libs/vr/libpdx/private/pdx/rpc/variant.h
+++ b/libs/vr/libpdx/private/pdx/rpc/variant.h
@@ -429,7 +429,7 @@
   // Handles assignment from the empty type. This overload supports assignment
   // in visitors using generic lambdas.
   Variant& operator=(EmptyVariant) {
-    Assign(EmptyVariant{});
+    Destruct();
     return *this;
   }
 
@@ -541,7 +541,10 @@
   void Construct(EmptyVariant) {}
 
   // Destroys the active element of the Variant.
-  void Destruct() { value_.Destruct(index_); }
+  void Destruct() {
+    value_.Destruct(index_);
+    index_ = kEmptyIndex;
+  }
 
   // Assigns the Variant when non-empty and the current type matches the target
   // type, otherwise destroys the current value and constructs a element of the
@@ -562,8 +565,6 @@
       Construct(std::forward<T>(value));
     }
   }
-  // Handles assignment from an empty Variant.
-  void Assign(EmptyVariant) { Destruct(); }
 };
 
 // Utility type to extract/convert values from a variant. This class simplifies

libs/vr/libpdx/variant_tests.cpp

diff --git a/libs/vr/libpdx/variant_tests.cpp b/libs/vr/libpdx/variant_tests.cpp
index 325f33f..b1ebc9b 100644
--- a/libs/vr/libpdx/variant_tests.cpp
+++ b/libs/vr/libpdx/variant_tests.cpp
@@ -584,6 +584,25 @@
     EXPECT_EQ(0u, InstrumentType<int>::move_assignment_count());
     EXPECT_EQ(1u, InstrumentType<int>::copy_assignment_count());
   }
+
+  {
+    InstrumentType<int>::clear();
+
+    // Construct from temporary, temporary ctor/dtor.
+    Variant<int, InstrumentType<int>> v(InstrumentType<int>(25));
+
+    // Assign EmptyVariant.
+    v = EmptyVariant{};
+
+    EXPECT_EQ(2u, InstrumentType<int>::constructor_count());
+    EXPECT_EQ(2u, InstrumentType<int>::destructor_count());
+    EXPECT_EQ(0u, InstrumentType<int>::move_assignment_count());
+    EXPECT_EQ(0u, InstrumentType<int>::copy_assignment_count());
+  }
+  EXPECT_EQ(2u, InstrumentType<int>::constructor_count());
+  EXPECT_EQ(2u, InstrumentType<int>::destructor_count());
+  EXPECT_EQ(0u, InstrumentType<int>::move_assignment_count());
+  EXPECT_EQ(0u, InstrumentType<int>::copy_assignment_count());
 }
 
 TEST(Variant, MoveConstructor) {
@@ -758,7 +777,7 @@
     Variant<std::string> b;
 
     std::swap(a, b);
-    EXPECT_TRUE(!a.empty());
+    EXPECT_TRUE(a.empty());
     EXPECT_TRUE(!b.empty());
     ASSERT_TRUE(b.is<std::string>());
     EXPECT_EQ("1", std::get<std::string>(b));
@@ -770,7 +789,7 @@
 
     std::swap(a, b);
     EXPECT_TRUE(!a.empty());
-    EXPECT_TRUE(!b.empty());
+    EXPECT_TRUE(b.empty());
     ASSERT_TRUE(a.is<std::string>());
     EXPECT_EQ("1", std::get<std::string>(a));
   }