8
\$\begingroup\$

Took a shot at implementing a subset of std::optional functionality. A lot of core features are there but some things like converting constructors, etc are missing as I just wanted to focus on the basic ideas.

The implementation:

#include <compare>
#include <typeinfo>
#include <utility>

struct BadOptionalAccess : std::exception {
    const char* what() const noexcept override {
        return "Tried to access an Optional()'s value, but no value exists!";
    }
};


template<typename T>
class Optional {
public:
    Optional() noexcept {
        std::memset(m_data, 0, sizeof(T));
    }

    Optional(const T& value) : 
        m_has_value{true} {
            new (m_data) T(value);
        }
    
    template<typename... Args>
    Optional(std::in_place_t, Args&&... args) {
        emplace(std::forward<Args>(args)...);
    }
    
    Optional(const Optional& other) : 
        m_has_value{other.m_has_value} {
            if (other.m_has_value) {
                new(m_data) T(other.value());
            }
        }
    
    Optional(Optional&& other) noexcept : 
        m_has_value{other.m_has_value} {
            if (other.has_value()) {
                new(m_data) T(std::move(other.value()));
                other.m_has_value = false;
            }
        }
    
    Optional& operator=(const Optional& other) {
        Optional temp {other};
        temp.swap(*this);
        return *this;
    }

    Optional& operator=(Optional&& other) noexcept {
        Optional temp {std::move(other)};
        temp.swap(*this);
        
        return *this;
    }

    auto operator<=>(const Optional& other) const noexcept {
        if (!has_value() && !other.has_value()) {
            return std::strong_ordering::equal;
        } else if (!has_value() && other.has_value()) {
            return std::strong_ordering::less;
        } else if (has_value() && !other.has_value()) {
            return std::strong_ordering::greater;
        } else {
            return value() <=> other.value();
        }
    }

    bool operator==(const Optional& other) const noexcept {
        return (*this <=> other) == std::strong_ordering::equal;
    }

    bool operator!=(const Optional& other) const noexcept {
        return !(*this == other);
    }

    explicit operator bool() const noexcept {
        return has_value();
    }
    
    template<typename... Args>
    void emplace(Args&&... args) {
        reset();
        new (m_data) T(std::forward<Args>(args)...);
        m_has_value = true;
    }

    template<typename U>
    T value_or(U&& default_value) {
        return has_value() ? std::move(**this) 
            : static_cast<T>(std::forward<U>(default_value));
    }

    void reset() {
        if (m_has_value) {
            T* val = reinterpret_cast<T*>(m_data);
            val->~T();
            m_has_value = false;
        }
    }

    void swap(Optional& other) noexcept {
        // Do I need to do this? My first instinct was to do
        // an ordinary swap of the m_data's, but it seemed wrong 
        // to do in case the types weren't trivially copyable/movable.
        // Maybe I should special case in those situations?
        if (other.has_value() && has_value()) {
            std::swap(*reinterpret_cast<T*>(m_data), *reinterpret_cast<T*>(other.m_data));
        } else if (other.has_value()) {
            new (m_data) T(std::move(other.value()));
        } else if (has_value()) {
            new(other.m_data) T(std::move(value()));
        }
        
        std::swap(m_has_value, other.m_has_value);
    }

    bool has_value() const noexcept {
        return m_has_value;
    }

    const T& value() const {
        return *ptr();
    }

    const T& operator*() const& {
        return value();
    }

    const T* operator->() const {
        return ptr();
    }

    T& value() {
        return *ptr();
    }

    T& operator*() & {
        return value();
    }

    T&& operator*() && noexcept {
        return std::move(value());
    }

    const T&& operator*() const&& noexcept {
        // not 100% sure how to use/test this overload. Why would an rvalue be const?
        return std::move(**this);
    }

    T* operator->() {
        return ptr();
    }

    ~Optional() {
        reset();
    }
        

private:
    alignas(T) unsigned char m_data[sizeof(T)];
    bool m_has_value = false;

    const T* ptr() const {
        if (!m_has_value) {
            throw BadOptionalAccess();
        }

        return (reinterpret_cast<const T*>(m_data));
    }

    T* ptr() {
        if (!m_has_value) {
            throw BadOptionalAccess();
        }

        return (reinterpret_cast<T*>(m_data));
    }
};

template<typename T, typename... Args>
Optional<T> makeOptional(Args&&... args) {
    return Optional<T>(std::in_place, std::forward<Args>(args)...);
}

Some tests:

#define BOOST_TEST_MODULE optionaltest

#ifdef BOOST_TEST_DYN_LINK
#include <boost/test/unit_test.hpp>
#else
#include <boost/test/included/unit_test.hpp>
#endif // BOOST_TEST_DYN_LINK

#include <boost/test/data/monomorphic.hpp>
#include <boost/test/data/test_case.hpp>

#include "Optional.h"
#include <string>



BOOST_AUTO_TEST_CASE(default_constructor_test) {
    Optional<int> o;
}

BOOST_AUTO_TEST_CASE(basic_constructors_test) {
     Optional<int> o1;
     Optional<int> o2 = 1;
     Optional<int> o3 = o2;
 
    // calls std::string( size_type count, CharT ch ) constructor
    Optional<std::string> o4(std::in_place, 3, 'A');
    Optional<std::string> o5 = makeOptional<std::string>(3, 'A');

    BOOST_CHECK(o4==o5);
 
    // Move-constructed from std::string using deduction guide to pick the type
    Optional o6(std::string{"deduction very long type"});
}

BOOST_AUTO_TEST_CASE(access_operator_test) {
    using namespace std::string_literals;

    Optional<int> opt1 = 1;
    BOOST_CHECK_EQUAL(*opt1, 1);
 
    *opt1 = 2;
    BOOST_CHECK_EQUAL(*opt1, 2);
 
    Optional<std::string> opt2 = "abc"s;
    BOOST_CHECK_EQUAL(*opt2, "abc"s);
    BOOST_CHECK_EQUAL(opt2->size(), 3); 
 
    Optional<std::string> taken = std::move(opt2);
    BOOST_CHECK_EQUAL(*taken, "abc"s); 
    BOOST_CHECK_EQUAL(taken->size(), 3); 

    BOOST_CHECK(!opt2.has_value()); 
}

BOOST_AUTO_TEST_CASE(value_check_test) {
    Optional<int> opt;
    BOOST_CHECK(!opt.has_value());
 
    opt = 43;
    BOOST_CHECK(opt.has_value());

    if (opt) {
        BOOST_CHECK(true);
    } else {
        BOOST_CHECK(false);
    }

    opt.reset();
    BOOST_CHECK(!opt.has_value());
}


BOOST_AUTO_TEST_CASE(get_value_test) {
    Optional<int> opt = {};

    BOOST_CHECK_THROW(opt.value(), BadOptionalAccess);
    BOOST_CHECK_THROW(opt.value() = 42, BadOptionalAccess);

    opt = 43;
    BOOST_CHECK_EQUAL(*opt, 43);
    BOOST_CHECK_EQUAL(opt.value(), 43);
    
    opt.value() = 44;
    BOOST_CHECK_EQUAL(*opt, 44);
    BOOST_CHECK_EQUAL(opt.value(), 44); 
}

BOOST_AUTO_TEST_CASE(ValueOrTest) {
    // Test with existing value
    Optional<int> opt1(5);
    BOOST_CHECK_EQUAL(opt1.value_or(10), 5);

    // Test without an existing value
    Optional<int> opt2; // Empty optional
    BOOST_CHECK_EQUAL(opt2.value_or(10), 10);

    // Test with a different type
    Optional<double> opt3; // Empty optional
    BOOST_CHECK_CLOSE(opt3.value_or(10), 10.0, 0.0001); // BOOST_CHECK_CLOSE for floating point comparison

    // Test with rvalue when Optional has a value
    Optional<std::string> opt4("Hello");
    BOOST_CHECK_EQUAL(opt4.value_or(std::string("World")), "Hello");

    // Test with rvalue when Optional does not have a value
    Optional<std::string> opt5; // Empty optional
    BOOST_CHECK_EQUAL(opt5.value_or(std::string("World")), "World");
}

BOOST_AUTO_TEST_CASE(swap_test) {
    Optional<std::string> opt1("Lorem ipsum dolor sit amet, consectetur tincidunt.");
    Optional<std::string> opt2("Some other lorem ipsum");

    opt1.swap(opt2);
    BOOST_CHECK_EQUAL(*opt2, "Lorem ipsum dolor sit amet, consectetur tincidunt.");
    BOOST_CHECK_EQUAL(*opt1, "Some other lorem ipsum");

    opt1.reset();
    opt1.swap(opt2);

    BOOST_CHECK_EQUAL(*opt1, "Lorem ipsum dolor sit amet, consectetur tincidunt.");
    BOOST_CHECK(!opt2.has_value());
}

BOOST_AUTO_TEST_CASE(comparison_test) {
    Optional<int> o1, o2;
    BOOST_CHECK(o1 == o2);

    o2.emplace(10);
    BOOST_CHECK(o1 < o2);
    BOOST_CHECK(o2 > o1);
    BOOST_CHECK(o2 != o1);

    o1.emplace(10);
    BOOST_CHECK(o1 == o2);
    BOOST_CHECK(o1 <= o2);
    BOOST_CHECK(o1 >= o2);
}
```
\$\endgroup\$

3 Answers 3

6
\$\begingroup\$

Missing #includes

You are missing #include <cstring> for std::memset(), and #include <new> for the placement new operator.

The converting constructors are a core feature

You have a non-converting constructor, but that does not even exist in std::optional. I would say that this is a core feature. Without it, you will get apparently inconsistent and surprising behavior. Consider:

Optional<int> foo = 3.1415;                  // works
Optional<float> bar = 42;                    // works
Optional<std::string> baz = "Hello, world!"; // fails to compile

The first two statements will happily cause conversions to happen. Fixing this so the third statement also compiles is trivial:

Optional(const auto& value) : 
    m_has_value{true} {
        new (m_data) T(value);
    }

I am very surprised by the choice of "basic features"; you have implemented operator<=>(), but I have never seen any code using the relational operators on std::optionals.

Unnecessary zeroing of memory in the default constructor

The default constructor does not need to zero m_data[], it's just going to cost you performance.

The move constructor should not unset other.m_has_value

In the move constructor, you set other.m_has_value to false, but you don't call the destructor of the other's value. And since the destructor of other will then not destruct its m_has_value, you will have created an object which will never be destructed propery.

You must leave other in a state such that everything will eventually be correctly destructed. However, as Igor G mentioned, std::optional's specification says that after a move construction, other.has_value() should not return a different value than before the move construction. So the best thing to do is to just not set other.m_has_value = false at all.

Incorrect behavior of swap()

Related to the above: you have a similar problem in swap(), in case you swap an optional which has a value with one which hasn't. Here you have to explicitly call reset() on the side which will no longer have a value after the swap.

About swapping

// Do I need to do this? My first instinct was to do
// an ordinary swap of the m_data's, but it seemed wrong 
// to do in case the types weren't trivially copyable/movable.
// Maybe I should special case in those situations?

You did the right thing in the code. You can't just swap the m_datas, that would just swap bytes without checking if that is legal, and would also bypass any specializations of std::swap for T. And in case T is trivially copyable/movable, then std::swap<T> would do exactly the same as std::swap<decltype<m_data>>, so there is no need to make this a special case.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Thanks @G. Sliepen for the help! As for how I chose "basic features", I just went onto cppreference.com, looked at the API, and tried to implement most of those things (but not necessarily all possible overloads of them), but I can see how not having a converting constructor was an oversight. \$\endgroup\$
    – jdav22
    Sep 18, 2023 at 22:52
  • 1
    \$\begingroup\$ @jdav22 You probably also need to implement a constructor that takes anything by r-value reference: Optional(auto&& value). Those are converting constructors. \$\endgroup\$
    – G. Sliepen
    Sep 19, 2023 at 6:43
  • 1
    \$\begingroup\$ @jdav22, "Very useful that it checks if the conversion is possible" -- I'm afraid there could be some misunderstanding here. The prototype Optional(const auto&) in itself doesn't check for any conversion. It does what it says: it accepts a single argument of any type. Try std::is_convertible<int, Optional<std::string>>::value and see that it's true. You must write explicit requirements to enable this constructor only for compatible argument types. \$\endgroup\$
    – Igor G
    Sep 19, 2023 at 9:10
  • 1
    \$\begingroup\$ @IgorG I see what you mean. I made the comment because I tried Optional<std::string> bar = 7; which failed with a compiler error, but the error is no matching function for call to ‘std::__cxx11::basic_string<char>::basic_string(const int&). So it's not that the converting constructor constrains this, rather std::string has no constructor which accepts an int. \$\endgroup\$
    – jdav22
    Sep 19, 2023 at 21:04
  • 1
    \$\begingroup\$ However, I think the (const auto&) constructor is then too lenient. After some searching I think something like this gives me the level of constraint I want: template<typename U> requires(std::is_constructible_v<T, const U&> && !std::is_same_v<std::decay_t<U>, T> && !std::is_same_v<std::decay_t<U>, Optional<T>>) Optional(const U& value); This also makes static_assert(std::is_convertible<int, Optional<std::string>>::value) false, which is what I would have expected. It's only true with the overly lenient converting constructor. \$\endgroup\$
    – jdav22
    Sep 19, 2023 at 21:07
5
\$\begingroup\$

Casting a storage to an object requires std::launder()

Since this question is tagged C++20, lines like this one:

return (reinterpret_cast<T*>(m_data));

should really be

return std::launder(reinterpret_cast<T*>(m_data));

Don't forget to set m_has_value every time you construct a new object.

Construction of T in m_data happens in many places, and sometimes it isn't accompanied by setting m_has_value to true. This kind of errors can be avoided if all construction was factored out to a single private function.

} else if (other.has_value()) {
    new (m_data) T(std::move(other.value()));
    // Hey, the object is there, but m_has_value is still false!
} else if ........

If moved-from optional contained some value before the move, it must still contain a value after the move

If an Optional contains a value, then every move should be performed on that contained value only.

Optional<int>      src = 42;
Optional<int>      dst(std::move(src));
assert( src.has_value() );

So, this line should be removed from constructor:

other.m_has_value = false;    // Wrong!

operator* and operator-> must be noexcept

Which means they cannot throw. Hence, they may not call neither value() nor ptr().

T& operator*() & {
    return value();   // That function may throw!
}

Don't underestimate SFINAE

Optional constructors and operators must participate in overload resolution only if certain conditions are met (read: they must be heavily loaded with SFINAE code checking those conditions). Getting all those conditions right isn't easy and requires lots of tests.

\$\endgroup\$
6
  • \$\begingroup\$ Yes, not setting other.m_has_value = false is also a valid way to move from. The main point is that the contents of other don't really matter, as long as it is in a valid enough state so other and its members are destroyed correctly at some point. \$\endgroup\$
    – G. Sliepen
    Sep 19, 2023 at 6:47
  • 1
    \$\begingroup\$ @G.Sliepen, I thought that this behavior is required by the Standard, note "rhs.has_value() is unchanged" requirement in eel.is/c++draft/optional.ctor#9 \$\endgroup\$
    – Igor G
    Sep 19, 2023 at 8:42
  • \$\begingroup\$ Ah! I didn't know it was part of std::optional's spec. I wonder why they did this though, normally when you move from an object, you shouldn't access it anymore anyway, so you'd think that the result of has_value() shouldn't matter. \$\endgroup\$
    – G. Sliepen
    Sep 19, 2023 at 9:10
  • \$\begingroup\$ @G.Sliepen, "when you move from an object, you shouldn't access it anymore" -- that's generally true, but as a special case of "access" you're allowed to assign new value to a moved-from object. That's what vector<movable_type> does all the time :- ) So my guess is that such behavior was specified to make optional_a = move(optional_b); optional_b = move(optional_c); as close to a = move(b); b = move(c) as possible. Just guessing. \$\endgroup\$
    – Igor G
    Sep 19, 2023 at 9:17
  • 1
    \$\begingroup\$ @jdav22, "when you move the value from rhs and rhs gets destroyed later on, then only the moved-from state of T is destroyed, guaranteeing you don't destroy something twice" -- yes, exactly. \$\endgroup\$
    – Igor G
    Sep 19, 2023 at 22:02
3
\$\begingroup\$

You have a lot of repeated, low level code. Create some private unsafe_ methods to avoid that repetition.

template<class...Ts>
void unsafe_emplace(Ts&&...ts);
void unsafe_destroy();
T* unsafe_get();
T const* unsafe_get() const;

these contain your reintepret_casts, std::launders, ::new( (void*)m_data ), ->~T()s.

They do absolutely no checking - that is what unsafe_ is for - and they are private.

Now your public methods:

void reset() {
  if (m_has_value) {
    unsafe_destroy();
  }
}

no longer fiddle around with the low-level operations.

The only "safe" thing the unsafe_ functions do is assign to m_has_value for destroy/emplace. They do not check the value before they operate.

And they work together:

void unsafe_destroy() {
  unsafe_get()->~T();
  m_has_value = false;
}

the reason I skip checking m_has_value is that reasonably often you'll know if it is true or not prior to calling these operations, and what to do when you get it wrong is either complex or nonsense.

Adding asserts is good however:

void unsafe_destroy() {
  assert(m_has_value);
  unsafe_get()->~T();
  m_has_value = false;
}

a second benefit is that when you are using placement new, you want to use ::new and cast its argument to void* to avoid the possibility that you hit an accidental manual override. By having your placement new code be in exactly one spot, fixing it in a bunch of spots stops being a problem.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks @Yakk for the help! Didn't think about how people's overrides of the new operator could do finicky things to mess with placement new, good thing to keep in mind. \$\endgroup\$
    – jdav22
    Sep 19, 2023 at 21:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.