Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Commit

Permalink
[core] Fix issues in unique_any
Browse files Browse the repository at this point in the history
* Eliminate unnecessary temporary in VTableStack::move, which also fixes calling the destructor on the incorrect instance
* Make move consistent: it destructs the src, not the dest, which is always empty
* delete doesn't need a null guard
* Conversions to void* don't need a cast
  • Loading branch information
jfirebaugh committed Jun 25, 2018
1 parent f3341dd commit d176e9b
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 14 deletions.
16 changes: 6 additions & 10 deletions include/mbgl/util/unique_any.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,12 @@ class unique_any final
template <typename ValueType>
struct VTableHeap : public VTable {
void move(Storage&& src, Storage& dest) override {
destroy(dest);
dest.dynamic = src.dynamic;
src.dynamic = nullptr;
}

void destroy(Storage& s) override {
if (s.dynamic) {
delete reinterpret_cast<ValueType*>(s.dynamic);
}
s.dynamic = nullptr;
delete reinterpret_cast<ValueType*>(s.dynamic);
}

const std::type_info& type() override {
Expand All @@ -154,9 +151,8 @@ class unique_any final
template <typename ValueType>
struct VTableStack : public VTable {
void move(Storage&& src, Storage& dest) override {
auto srcValue = reinterpret_cast<ValueType&&>(src.stack);
new (static_cast<void*>(&dest.stack)) ValueType(std::move(srcValue));
srcValue.~ValueType();
new (&dest.stack) ValueType(std::move(reinterpret_cast<ValueType&>(src.stack)));
destroy(src);
}

void destroy(Storage& s) override {
Expand All @@ -178,13 +174,13 @@ class unique_any final
template <typename ValueType, typename _Vt>
std::enable_if_t<AllocateOnStack<_Vt>::value>
createStorage(ValueType&& value) {
new (static_cast<void*>(&storage.stack)) _Vt(std::forward<ValueType>(value));
new (&storage.stack) _Vt(std::forward<ValueType>(value));
}

template <typename ValueType, typename _Vt>
std::enable_if_t<!AllocateOnStack<_Vt>::value>
createStorage(ValueType&& value) {
storage.dynamic = static_cast<void*>(new _Vt(std::forward<ValueType>(value)));
storage.dynamic = new _Vt(std::forward<ValueType>(value));
}

template <typename ValueType>
Expand Down
40 changes: 36 additions & 4 deletions test/util/unique_any.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ class TestType {
str[0] = 'a';
}

TestType(unique_any& p) : TestType() {
p = std::unique_ptr<TestType>(this);
}

//Detect moves
TestType(TestType&& t): i1(t.i1+1), i2(t.i2+2) {
str[0] = t.str[0]+1;
}

TestType(const TestType&) = delete;
TestType& operator=(const TestType&) = delete;

int i1;
int i2;
char str[256];
Expand Down Expand Up @@ -88,6 +87,39 @@ TEST(UniqueAny, BasicTypes_Move) {

}

TEST(UniqueAny, SmallType) {
struct T {
T(int32_t* p_) : p(p_) {
(*p)++;
}

T(T&& t) noexcept : p(t.p) {
(*p)++;
}

~T() {
(*p)--;
}

T(const T&) = delete;
T& operator=(const T&) = delete;

int32_t* p;
};

int32_t p = 0;

{
unique_any u1 = unique_any(T(&p));
EXPECT_EQ(p, 1);

auto u2(std::move(u1));
EXPECT_EQ(p, 1);
}

EXPECT_EQ(p, 0);
}

TEST(UniqueAny, LargeType) {
TestType t1;
unique_any u1 = unique_any(std::move(t1));
Expand Down

0 comments on commit d176e9b

Please sign in to comment.