Skip to content

Commit

Permalink
fix: fix move construct and move assign for Variant (#749)
Browse files Browse the repository at this point in the history 
* fix: fix move construct and move assign for Variant

* Fixes for GCC
  • Loading branch information
@richardapeters
richardapeters authored Oct 24, 2024
1 parent 66e4f4a commit ed39658
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 16 deletions.
36 changes: 28 additions & 8 deletions infra/util/Variant.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace infra

Variant();
Variant(const Variant& other);
Variant(Variant&& other) noexcept((std::is_nothrow_move_constructible_v<T> && ...)) = default;
Variant(Variant&& other) noexcept((std::is_nothrow_move_constructible_v<T> && ...));
template<class... T2>
Variant(const Variant<T2...>& other);
template<class U>
Expand All @@ -31,7 +31,7 @@ namespace infra
Variant(AtIndex, std::size_t index, Args&&... args);

Variant& operator=(const Variant& other);
Variant& operator=(Variant&& other) noexcept((std::is_nothrow_move_assignable_v<T> && ...) && (std::is_nothrow_move_constructible_v<T> && ...)) = default;
Variant& operator=(Variant&& other) noexcept((std::is_nothrow_move_assignable_v<T> && ...) && (std::is_nothrow_move_constructible_v<T> && ...));
template<class... T2>
Variant& operator=(const Variant<T2...>& other);
template<class U>
Expand Down Expand Up @@ -82,15 +82,16 @@ namespace infra
template<class... Args>
void ConstructByIndexInEmptyVariant(std::size_t index, Args&&... args);

private:
void Destruct();

private:
std::size_t dataIndex = 0;
typename std::aligned_storage<MaxSizeOfTypes<T...>::value, MaxAlignmentOfTypes<T...>::value>::type data;

template<class... T2>
friend struct detail::ConstructVisitor;
friend struct detail::CopyConstructVisitor;
template<class... T2>
friend struct detail::MoveConstructVisitor;
};

template<class... T>
Expand All @@ -114,15 +115,22 @@ namespace infra
template<class... T>
Variant<T...>::Variant(const Variant& other)
{
detail::ConstructVisitor<T...> visitor(*this);
detail::CopyConstructVisitor<T...> visitor(*this);
ApplyVisitor(visitor, other);
}

template<class... T>
Variant<T...>::Variant(Variant&& other) noexcept((std::is_nothrow_move_constructible_v<T> && ...))
{
detail::MoveConstructVisitor<T...> visitor(*this);
ApplyVisitor(visitor, other);
}

template<class... T>
template<class... T2>
Variant<T...>::Variant(const Variant<T2...>& other)
{
detail::ConstructVisitor<T...> visitor(*this);
detail::CopyConstructVisitor<T...> visitor(*this);
ApplyVisitor(visitor, other);
}

Expand Down Expand Up @@ -152,7 +160,19 @@ namespace infra
{
if (this != &other)
{
detail::CopyVisitor<T...> visitor(*this);
detail::CopyAssignVisitor<T...> visitor(*this);
ApplyVisitor(visitor, other);
}

return *this;
}

template<class... T>
Variant<T...>& Variant<T...>::operator=(Variant&& other) noexcept((std::is_nothrow_move_assignable_v<T> && ...) && (std::is_nothrow_move_constructible_v<T> && ...))
{
if (this != &other)
{
detail::MoveAssignVisitor<T...> visitor(*this);
ApplyVisitor(visitor, other);
}

Expand All @@ -163,7 +183,7 @@ namespace infra
template<class... T2>
Variant<T...>& Variant<T...>::operator=(const Variant<T2...>& other)
{
detail::CopyVisitor<T...> visitor(*this);
detail::CopyAssignVisitor<T...> visitor(*this);
ApplyVisitor(visitor, other);

return *this;
Expand Down
67 changes: 59 additions & 8 deletions infra/util/VariantDetail.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ namespace infra
};

template<class... T>
struct ConstructVisitor
struct CopyConstructVisitor
: StaticVisitor<void>
{
explicit ConstructVisitor(Variant<T...>& aVariant);
explicit CopyConstructVisitor(Variant<T...>& aVariant);

template<class T2>
void operator()(const T2& v);
Expand All @@ -63,10 +63,23 @@ namespace infra
};

template<class... T>
struct CopyVisitor
struct MoveConstructVisitor
: StaticVisitor<void>
{
explicit CopyVisitor(Variant<T...>& aVariant);
explicit MoveConstructVisitor(Variant<T...>& aVariant);

template<class T2>
void operator()(T2& v);

private:
Variant<T...>& variant;
};

template<class... T>
struct CopyAssignVisitor
: StaticVisitor<void>
{
explicit CopyAssignVisitor(Variant<T...>& aVariant);

template<class T2>
void operator()(const T2& v);
Expand All @@ -75,6 +88,19 @@ namespace infra
Variant<T...>& variant;
};

template<class... T>
struct MoveAssignVisitor
: StaticVisitor<void>
{
explicit MoveAssignVisitor(Variant<T...>& aVariant);

template<class T2>
void operator()(T2& v);

private:
Variant<T...>& variant;
};

struct DestroyVisitor
: StaticVisitor<void>
{
Expand Down Expand Up @@ -504,29 +530,54 @@ namespace infra
}

template<class... T>
ConstructVisitor<T...>::ConstructVisitor(Variant<T...>& aVariant)
CopyConstructVisitor<T...>::CopyConstructVisitor(Variant<T...>& aVariant)
: variant(aVariant)
{}

template<class... T>
template<class T2>
void ConstructVisitor<T...>::operator()(const T2& v)
void CopyConstructVisitor<T...>::operator()(const T2& v)
{
variant.template ConstructInEmptyVariant<T2>(v);
}

template<class... T>
CopyVisitor<T...>::CopyVisitor(Variant<T...>& aVariant)
MoveConstructVisitor<T...>::MoveConstructVisitor(Variant<T...>& aVariant)
: variant(aVariant)
{}

template<class... T>
template<class T2>
void MoveConstructVisitor<T...>::operator()(T2& v)
{
variant.template ConstructInEmptyVariant<T2>(std::move(v));
}

template<class... T>
CopyAssignVisitor<T...>::CopyAssignVisitor(Variant<T...>& aVariant)
: variant(aVariant)
{}

template<class... T>
template<class T2>
void CopyVisitor<T...>::operator()(const T2& v)
void CopyAssignVisitor<T...>::operator()(const T2& v)
{
variant = v;
}

template<class... T>
MoveAssignVisitor<T...>::MoveAssignVisitor(Variant<T...>& aVariant)
: variant(aVariant)
{}

template<class... T>
template<class T2>
void MoveAssignVisitor<T...>::operator()(T2& v)
{
variant.Destruct();
variant.template ConstructInEmptyVariant<T2>(std::move(v));
}

template<class T>
bool EqualVisitor::operator()(const T& x, const T& y) const
{
Expand Down
45 changes: 45 additions & 0 deletions infra/util/test/TestVariant.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,32 @@
#include "gtest/gtest.h"
#include <cstdint>

struct MoveableStruct
{
MoveableStruct(int x)
: x(x)
{}

MoveableStruct(const MoveableStruct& other) = delete;
MoveableStruct& operator=(const MoveableStruct& other) = delete;

MoveableStruct(MoveableStruct&& other)
: x(other.x)
{
other.x = 0;
}

MoveableStruct& operator=(MoveableStruct&& other)
{
x = other.x;
other.x = 0;

return *this;
}

int x;
};

TEST(VariantTest, TestEmptyConstruction)
{
infra::Variant<bool> v;
Expand Down Expand Up @@ -111,6 +137,25 @@ TEST(VariantTest, TestAssignmentFromNarrowVariant)
EXPECT_EQ(5, v.Get<int>());
}

TEST(VariantTest, TestMoveConstruct)
{
infra::Variant<MoveableStruct> v(infra::InPlaceType<MoveableStruct>(), 2);
auto v2(std::move(v));
EXPECT_EQ(0, v2.Which());
EXPECT_EQ(0, v.Get<MoveableStruct>().x);
EXPECT_EQ(2, v2.Get<MoveableStruct>().x);
}

TEST(VariantTest, TestMoveAssign)
{
infra::Variant<MoveableStruct> v(infra::InPlaceType<MoveableStruct>(), 2);
infra::Variant<MoveableStruct> v2(infra::InPlaceType<MoveableStruct>(), 3);
v2 = std::move(v);
EXPECT_EQ(0, v2.Which());
EXPECT_EQ(0, v.Get<MoveableStruct>().x);
EXPECT_EQ(2, v2.Get<MoveableStruct>().x);
}

TEST(VariantTest, TestVariantWithTwoTypes)
{
int i = 5;
Expand Down

0 comments on commit ed39658

Please sign in to comment.