Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit dfee6a7b authored by Corey Tabaka's avatar Corey Tabaka
Browse files

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
parent 5d5c530a

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

+5 −4
Original line number Diff line number Diff line
@@ -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

libs/vr/libpdx/variant_tests.cpp

+21 −2
Original line number Diff line number Diff line
@@ -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));

  }