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

diff --git a/libs/vr/libpdx/private/pdx/rpc/variant.h b/libs/vr/libpdx/private/pdx/rpc/variant.h
index cb44a51..dc321fa 100644 (file)
--- a/libs/vr/libpdx/private/pdx/rpc/variant.h
+++ b/libs/vr/libpdx/private/pdx/rpc/variant.h
@@ -429,7 +429,7 @@ class Variant {
   // 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 @@ class Variant {
   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 @@ class Variant {
       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

diff --git a/libs/vr/libpdx/variant_tests.cpp b/libs/vr/libpdx/variant_tests.cpp
index 325f33f..b1ebc9b 100644 (file)
--- a/libs/vr/libpdx/variant_tests.cpp
+++ b/libs/vr/libpdx/variant_tests.cpp
@@ -584,6 +584,25 @@ TEST(Variant, CopyMoveConstructAssign) {
     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 @@ TEST(Variant, Swap) {
     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 @@ TEST(Variant, Swap) {
 
     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));
   }