2
\$\begingroup\$

This is a first version of an implementation of std::optional it is supposed to compile under C++14. The public interface of the class is complete, but there are still quite a few things missing. Only a few of the constructor availability traits are checked, none of the noexcept clauses are implemented, no non-member functions are implemented. Also I saw that most implementations that are out there split the storage and the public interface into two separate classes, or inherit from a base class. I wanted to get a baseline implementation working and tested and then move forward with maybe better abstractions internally.

What is there has been unit tested for most code-paths, or manually checked, some of the constraints are hard to verify. E.g. how to verify that a destructor was not called when the object is trivially destructable.

I am also still puzzling about a few of the signatures e.g.

    constexpr const T&& operator*() const&& noexcept { return std::move(*reinterpret_cast<const T*>(&storage_)); }

This seems to silently discard const by letting the user move the content out of the optional (if it is an rvalue).

The current code, including tests is available at (https://github.com/HarryDC/optional), I'm reinventing the wheel here for educational purposes, this touches a lot of areas that just don't come up in my normal use of C++. This was developed under Visual Studio, and spot checked on compiler explorer under different compilers.

#include <exception>
#include <initializer_list>
#include <utility>

namespace hs
{

// Missing from C++14
template< class From, class To >
constexpr bool is_convertible_v = std::is_convertible<From, To>::value;

template<class A, class B>
constexpr bool is_same_v = std::is_same<A, B>::value;

// Internals
namespace detail
{
template < typename T, typename std::enable_if_t<std::is_trivially_destructible<T>::value, int> = 0>
void destruct(T*) {}

template < typename T, typename std::enable_if_t < !std::is_trivially_destructible<T>::value, int > = 0 >
void destruct(T* t)
{
    t->~T();
}


} // namespace detail

// Optional types
class bad_optional_access : public std::exception {};

struct nullopt_t
{
    explicit nullopt_t() = default;
};
constexpr nullopt_t nullopt{};

struct in_place_t
{
    explicit in_place_t() = default;
};
constexpr in_place_t in_place{};

// Public Class
template <class T>
class optional
{
public:
    using value_type = T;

    // Constructors

    constexpr optional() noexcept = default;
    constexpr optional(nullopt_t) noexcept {}

    constexpr optional(const optional& other)
    {
        if (!other.has_value_) return;
        new (&storage_) T(*other);
        has_value_ = true;
    }

    constexpr optional(optional&& other)
    {
        if (!other.has_value_) return;
        new (&storage_) T(std::move(*other));
        has_value_ = true;
    }

    template < class U >
    optional(const optional<U>& other)
    {
        if (!other.has_value()) return;
        new (&storage_) T(*other);
        has_value_ = true;
    }

    template < class U >
    optional(optional<U>&& other)
    {
        if (!other.has_value()) return;
        new (&storage_) T(std::move(*other));
        has_value_ = true;
    }

    template< class... Args >
    constexpr explicit optional(in_place_t, Args&& ... args)
    {
        new (&storage_) T(std::forward<Args>(args)...);
        has_value_ = true;
    }

    template< class U, class... Args >
    constexpr explicit optional(hs::in_place_t,
                                std::initializer_list<U> ilist,
                                Args&& ... args)
    {
        new (&storage_) T(std::forward<std::initializer_list<U>>(ilist), std::forward<Args>(args)...);
        has_value_ = true;
    }

    template < class U = value_type,
               typename std::enable_if_t < is_convertible_v<U, T>&&
                                           !is_same_v<std::decay_t<U>, optional<T>>, int > = 0
               >
    constexpr optional(U && val)
    {
        new (&storage_) T(std::forward<U>(val));
        has_value_ = true;
    }

    // Destructor
    ~optional()
    {
        if (has_value_) detail::destruct<T>(reinterpret_cast<T*>(&storage_));
    }

    // Operator =
    optional& operator=(nullopt_t) noexcept
    {
        reset();
        return *this;
    }

    // Don't know why the following two overloads (2/3) are separate from copy-op 5/6
    constexpr optional& operator=(const optional& other)
    {
        if (other.has_value_)
        {
            if (has_value_)
            {
                **this = *other;
            }
            else
            {
                new (&storage_) T(*other);
                has_value_ = true;
            }
        }
        else
        {
            reset();
        }
        return *this;
    }

    constexpr optional& operator=(optional&& other) noexcept
    {
        if (other.has_value_)
        {
            if (has_value_)
            {
                **this = std::move(*other);
            }
            else
            {
                new (&storage_) T(std::move(*other));
                has_value_ = true;
            }
        }
        else
        {
            reset();
        }
        return *this;
    }

    template < class U = value_type,
               typename std::enable_if_t < is_convertible_v<U, T>&&
                                           !is_same_v<std::decay_t<U>, optional<T>>, int > = 0
               >
    optional & operator=(U && value)
    {
        if (has_value_)
        {
            **this = std::forward<U>(value);
        }
        else
        {
            new (&storage_) T(std::forward<U>(value));
            has_value_ = true;
        }
        return *this;
    }

    template< class U >
    optional& operator=(const optional<U>& other)
    {
        if (other.has_value())
        {
            if (has_value_)
            {
                **this = *other;
            }
            else
            {
                new (&storage_) T(*other);
                has_value_ = true;
            }
        }
        else
        {
            reset();
        }
        return *this;
    }

    template< class U >
    optional& operator=(optional<U>&& other)
    {
        if (other.has_value())
        {
            if (has_value_)
            {
                **this = std::move(*other);
            }
            else
            {
                new (&storage_) T(std::move(*other));
                has_value_ = true;
            }
        }
        else
        {
            reset();
        }
        return *this;
    }

    // Operator ->, *
    // TODO unit test ->

    constexpr T* operator->() noexcept { return reinterpret_cast<T*>(&storage_); }
    constexpr const T* operator->() const noexcept { return reinterpret_cast<const T*>(&storage_); }

    constexpr T& operator*()& noexcept { return *reinterpret_cast<T*>(&storage_); }
    constexpr const T& operator*()const& noexcept { return *reinterpret_cast<const T*>(&storage_); }

    constexpr T&& operator*()&& noexcept { return std::move(*reinterpret_cast<T*>(&storage_)); }

    // What does const in this context mean ??? How to test this
    constexpr const T&& operator*() const&& noexcept { return std::move(*reinterpret_cast<const T*>(&storage_)); }

    // operator bool, has_value()
    constexpr operator bool() const noexcept { return has_value_; }
    constexpr bool has_value() const noexcept { return has_value_; }

    // value()

    constexpr T& value()&
    {
        if (has_value_) return *reinterpret_cast<T*>(&storage_);
        else throw bad_optional_access();
    }

    constexpr const T& value() const&
    {
        if (has_value_) return *reinterpret_cast<const T*>(&storage_);
        else throw bad_optional_access();
    }

    // This is on an r-value Do we need to do anything different here ???
    constexpr T&& value()&&
    {
        if (has_value_) return std::move(*reinterpret_cast<T*>(&storage_));
        else throw bad_optional_access();
    }

    // This is on an r-value Do we need to do anything different here ???
    // TODO unittest (HOW ???)
    constexpr const T&& value() const&&
    {
        if (has_value_) return std::move(*reinterpret_cast<T*>(&storage_));
        else throw bad_optional_access();
    }

    // value_or()
    template <class U>
    constexpr T value_or(U&& default_value) const&
    {
        return (has_value_) ? (**this) : static_cast<T>(std::forward<U>(default_value));
    }

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

    // swap
    void swap(optional& other)
    {
        if (has_value_ && other)
        {
            std::swap(**this, *other);
        }
        else if (has_value_)
        {
            other = std::move(*this);
            reset();
        }
        else if (other)
        {
            *this = std::move(*other);
            other.reset();
        }
    }


    // reset
    void reset() noexcept
    {
        if (has_value_) detail::destruct<T>(reinterpret_cast<T*>(&storage_));
        has_value_ = false;
    }

    // emplace
    template< class... Args >
    T& emplace(Args&& ... args)
    {
        new (&storage_) T(std::forward<Args>(args)...);
        has_value_ = true;
        return **this;
    }

    template< class U, class... Args >
    T& emplace(std::initializer_list<U> ilist, Args&& ... args)
    {
        new (&storage_) T(std::forward<std::initializer_list<U>>(ilist), std::forward<Args>(args)...);
        has_value_ = true;
        return **this;
    }

private:
    bool has_value_{ false };
    typename std::aligned_storage<sizeof(T), alignof(T)>::type storage_;
};
// TBD ...
// Non-member func
// comparators
// make_optional
// std::swap

// Helper Class
// std::hash
}
```
\$\endgroup\$
  • \$\begingroup\$ "This seems to silently discard const by letting the user move the content out of the optional (if it is an rvalue)." Can you explain your reasoning for why this silently discards const? \$\endgroup\$ – Justin Apr 2 at 2:51
  • \$\begingroup\$ I think i'm basically just confused about the overall signature and its semantics (i.e. learning experience), its a const function, called on an rvalue reference. It returns a forwarding reference (not an rvalue reference) to the value contained inside the (if I am getting the nomenclature right here). By returning the value of the optional using std::move it is marked as an rvalue. I think i'm missing the purpose of this signature. Maybe the comment thread here is not the right place to discuss this either ... \$\endgroup\$ – Harald Scheirich Apr 2 at 12:24
3
\$\begingroup\$

See my A standard-conforming C++17 std::optional implementation, partly inspired by this post.


(Note: C++17-only features like inline variables are ignored in this answer.)

The cryptic const && signature

First, let me answer your question:

I am also still puzzling about a few of the signatures e.g.

constexpr const T&& operator*() const&& noexcept { return std::move(*reinterpret_cast<const T*>(&storage_)); }

This seems to silently discard const by letting the user move the content out of the optional (if it is an rvalue).

Good question. Your implementation is correct. optional is designed to be completely transparent with regard to value category, so that calling * on an rvalue optional returns an rvalue. Given that we allow &, const &, and && work correctly, there is not a reason to treat const && unfairly. A const && cannot actually be moved from.

I would implement it as

constexpr const T&& operator*() const&& noexcept
{
    return std::move(**this);
}

to reduce code duplication. Here, **this calls the const & overload because *this is always considered an lvalue expression. I have yet to see a practical use of this overload.

You can test it like this:

const optional<int> x{42};
static_assert(std::is_same<decltype(*std::move(x)), const int&&>::value);

Same for value().

constexpr friendliness

Your implementation is not constexpr friendly. Something as basic as:

constexpr hs::optional<int> x{42};

fails because your optional has a non-trivial destructor. Let's look up the definition of trivial destructor in C++14: ([class.dtor]/5, emphasis mine)

[...]

A destructor is trivial if it is not user-provided and if:

  • the destructor is not virtual,

  • all of the direct base classes of its class have trivial destructors, and

  • for all of the non-static data members of its class that are of class type (or array thereof), each such class has a trivial destructor.

Otherwise, the destructor is non-trivial.

Your destructor is user-provided, hence non-trivial.

The only way to properly implement a constexpr friendly optional, I suppose, is to use a union. That's how constexpr machinery work under the hood. And that also explains the connection between the constexpr-ness of the copy / move operations on optional and the trivially of the corresponding operations on the value type as specified in the standard.

destruct

(The verb is formally called "destroy" in C++, not "destruct", although the nouns are "destructor" and "destruction" and the adjective is "destructible".)

The destruct function exists to optimize out trivial destructor calls. However, a competent compiler should be able to optimize such calls on itself. Therefore, I suggest removing the function altogether.

nullopt_t

Per [optional.nullopt]/2:

Type nullopt_­t shall not have a default constructor or an initializer-list constructor, and shall not be an aggregate.

Your nullopt_t is default constructible. Simple fix:

struct nullopt_t {
    explicit constexpr nullopt_t(int) {}
};
constexpr nullopt_t nullopt{42};

Constructors

The copy constructor is not defined as deleted when it should. The move constructor is missing noexcept specification and participates in overload resolution when it shouldn't. Implementing the special member functions (copy/move constructor/assignment) correctly requires the use of base classes and template specialization (you don't want to duplicate the whole class just to dispatch on is_move_constructible).

(You may ask: can't we use SFINAE? For constructors, we can add default arguments; for assignment operators, we can play with the return type. The answer is no. SFINAE only with templates (member functions in a class template are not automatically templates), and the special member functions cannot be templates. If you write a templates as an attempt to implement them, the default (wrong) versions will still be generated and take precedence over the templates.)

This also affects the other constructors / constructor templates and their explicitness. They are easier to implement because SFINAE can be used. Usually, the way to implement conditional explicit before C++20 is to declare two constructors and use SFINAE to ensure that they do not participate in overload resolution at the same time.

Incidentally, your constructors repeat a lot of code. I suggest extracting a separate function to deal with construction: (note that you are not supposed to forward initializer_lists)

private:
    template <typename... Args>
    void construct(Args&&... args)
    {
        assert(!has_value);
        new (&storage_) T(std::forward<Args>(args)...);
        has_value_ = true;
    }

    template <typename U, typename... Args>
    void construct(std::initializer_list<U> init, Args&&... args)
    {
        assert(!has_value);
        new (&storage_) T(init, std::forward<Args>(args)...);
        has_value_ = true;
    }

and use it to simplify the constructors:

optional(const optional& other)
{
    if (!other)
        construct(*other);
}

optional(optional&& other)
{
    if (!other)
        construct(std::move(*other));
}

// etc.

Assignment

The copy/move assignment operators should also be defined as deleted / excluded from overload resolution as required by the standard. Similar for other assignment operators. See the previous point about copy/move constructors and other constructors.

// Don't know why the following two overloads (2/3) are separate from copy-op 5/6

Because the default versions of the copy assignment operator and move assignment operator automatically generate (as deleted) and take precedence over the templates if you don't implement them.

The logic of the assignment operators can probably be unified / simplified somehow. Something like:

template <typename U>
void construct_or_assign(U&& val)
{
    if (*this)
        **this = std::forward<U>(val);
    else
        construct(std::forward<U>(val));
}

(with apologies to Thomas Köppe [1] for stealing the name.)

Observers

The dereference operators look nice.

operator bool should be explicit.

// This is on an r-value Do we need to do anything different here ???

No, I think you are doing fine.

Emplace

emplace should call reset() before constructing the new element, or the original element will not be properly destroyed.

Miscellaneous

You are missing a few #includes (<type_traits>, <typeindex> for std::hash, etc.).

The typename before enable_if_t is redundant:

template <typename T, /*typename*/ std::enable_if_t<std::is_trivially_destructible<T>::value, int> = 0>
\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments, I haven't done much work on this but it seems the only solution to enabling the correct traits, like trivially constructable, and some of the overloads is building a chain of classes that will inherit the correct traits. Every implementation i've seen in the wild is doing that. Any other ways of accomplishing this ? \$\endgroup\$ – Harald Scheirich Aug 2 at 14:47
  • \$\begingroup\$ @HaraldScheirich I'm afraid not. The real problem is that the special member functions cannot be templates, and so SFINAE is not applicable. I cannot really think of another way to implement this other than to use base class template + template specialization. Maybe I am not creative enough, though. \$\endgroup\$ – L. F. Aug 3 at 3:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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