Skip to content

Commit

Permalink
Fix bugs in in ctl::optional (#1203)
Browse files Browse the repository at this point in the history
Manually manage the lifetime of `value_` by using an anonymous
`union`. This fixes a bunch of double-frees and double-constructs.

Additionally move the `present_` flag last. When `T` has padding
`present_` will be placed there saving `alignof(T)` bytes from
`sizeof(optional<T>)`.
  • Loading branch information
alkis authored Jun 8, 2024
1 parent 2ba6b01 commit d44a7dc
Showing 2 changed files with 32 additions and 12 deletions.
35 changes: 23 additions & 12 deletions ctl/optional.h
Original file line number Diff line number Diff line change
@@ -14,48 +14,56 @@ class optional
public:
using value_type = T;

~optional() = default;
~optional()
{
if (present_)
value_.~T();
}

optional() noexcept : present_(false)
{
}

optional(const T& value) : present_(true), value_(value)
optional(const T& value) : present_(true)
{
new (&value_) T(value);
}

optional(T&& value) : present_(true), value_(std::move(value))
optional(T&& value) : present_(true)
{
new (&value_) T(std::move(value));
}

optional(const optional& other) : present_(other.present_)
{
if (present_)
if (other.present_)
new (&value_) T(other.value_);
}

optional(optional&& other) noexcept : present_(other.present_)
{
if (present_)
value_ = std::move(other.value_);
if (other.present_)
new (&value_) T(std::move(other.value_));
}

optional& operator=(const optional& other)
{
if (this != &other) {
reset();
if (other.present_)
new (&value_) T(other.value_);
present_ = other.present_;
if (present_)
value_ = other.value_;
}
return *this;
}

optional& operator=(optional&& other) noexcept
{
if (this != &other) {
reset();
if (other.present_)
new (&value_) T(std::move(other.value_));
present_ = other.present_;
if (present_)
value_ = std::move(other.value_);
}
return *this;
}
@@ -102,8 +110,9 @@ class optional
template<typename... Args>
void emplace(Args&&... args)
{
reset();
present_ = true;
value_ = T(std::forward<Args>(args)...);
new (&value_) T(std::forward<Args>(args)...);
}

void swap(optional& other) noexcept
@@ -120,8 +129,10 @@ class optional
}

private:
union {
T value_;
};
bool present_;
T value_;
};

} // namespace ctl
9 changes: 9 additions & 0 deletions test/ctl/optional_test.cc
Original file line number Diff line number Diff line change
@@ -26,6 +26,8 @@
// #include <string>
// #define ctl std

static int g = 0;

int
main()
{
@@ -114,6 +116,13 @@ main()
return 24;
}

{
struct A { int* p = &g; A() {++*p; } };
ctl::optional<A> x;
if (g != 0)
return 25;
}

CheckForMemoryLeaks();
return 0;
}

0 comments on commit d44a7dc

Please sign in to comment.