An optional<T> implementation

This is an implementation of optional from C++17, it is mostly standard conforming.

I'm looking for a review on efficiency, correctness and performance.

Sample usage

#include "optional.h"

int main()
{
    util::optional s0{ "abc" };
    util::optional s1;

    if ( s1 ) s0 = s1;
    else s1 = s0;

    if ( s0 == s1 && s0 == std::string{ "abc" } )
    {
        s1 = "def";
        s0 = util::nullopt;
    }

    using std::swap;
    swap( s1, s0 );

    std::cout << *s0 << '\n';
    std::cout << s1.value_or( "default" ) << '\n';
}

Implementation

optional.h

```
#ifndef UTIL_OPTIONAL_H
#define UTIL_OPTIONAL_H

#include
#include
#include "optional_comparison.h"

namespace util
{
// 20.5.4, in-place construction
struct in_place_t {};
constexpr in_place_t in_place{};

// 20.5.5, no-value state indicator
struct nullopt_t
{
constexpr nullopt_t( int const ) noexcept {};
};
constexpr nullopt_t nullopt{ 0 };

// 20.5.6, class bad_optional_access
class bad_optional_access : public std::logic_error
{
public:
bad_optional_access() : logic_error( "uninitialized optional" ) {}
};

// 20.5.3, optional for object types
template
class optional
{
private:
struct conversion_ctor_t {};
static constexpr conversion_ctor_t conversion_ctor{};

public:
template friend class optional;

using value_type = T;

// 20.5.3.2, destructor
// issue: not trivially destructible if T is trivially destructible
~optional() noexcept( std::is_nothrow_destructible::value )
{
destroy_value();
}

// 20.5.3.1, constructors
// issue: vc++2015 update 2 cannot declare the following as constexpr:
// optional();
// optional( nullopt_t );
// optional( optional const& );
// optional( optional&& );
// optional( T const& );
// optional(

Bugs

Consider this:

T val;
optional o;
o = val;

This calls operator=(U&& value) with U = T&, which calls set_value(U&& value) with U = T&, which sets m_has_value to true then calls emplace()! This makes emplace() think that you actually have a value, and so destroys it. But we didn't have a value originally - so undefined behavior.

You have similar bugs in other places. emplace() is responsible for setting m_has_value() to true, nothing else.

In a similar vein, destroy_value() should set m_has_value to false, so that you don't have to manually do it in other places.

Constructors

This constructor needs SFINAE:

template 
constexpr optional( in_place_t, Args&&... args )

Otherwise you'll get the wrong thing from is_constructible.

This constructor seems wrong - what types are constructible from both an initializer_list and something else (I'm omitting the SFINAE for brevity)? Perhaps just the initializer_list?

template
constexpr optional( in_place_t, std::initializer_list il, Args&&... args )

Assignment

This cast is unnecessary:

if ( static_cast( this ) != &other )

Destructor

You don't have to check if is_trivially_destructible::value. If it were trivially destructible, the destructor just wouldn't do anything. So you can just write:

constexpr void destroy_value() noexcept(std::is_nothrow_destructible::value)
{
    if (m_has_value) {
        m_value.T::~T();
    }
}

StackExchange Code Review Q#127498, answer score: 5