martinmoene / optional-lite

optional lite - A C++17-like optional, a nullable object for C++98, C++11 and later in a single-file header-only library
Boost Software License 1.0
403 stars 45 forks source link

Compile error on implicit construction of T from another type #27

Closed yoursunny closed 6 years ago

yoursunny commented 6 years ago

Snippet to reproduce:

// g++ -std=c++14 -o x x.cpp

#include "optional.hpp"

class A
{
};

// B is implicitly constructible from A.
class B
{
public:
  B(A a)
  {
  }
};

// f accepts optional<B>.
void f(nonstd::optional<B> b)
{
}

// check conditions on https://en.cppreference.com/w/cpp/utility/optional/optional item 8
static_assert(std::is_constructible<B, A&&>::value, "");
static_assert(!std::is_same<std::decay_t<A&&>, nonstd::in_place_t>::value, "");
static_assert(!std::is_same<std::decay_t<A&&>, nonstd::optional<B>>::value, "");

int main()
{
  A a;

  f(a); // Call f with A, compile error.
}

This snippet fails to compile on gcc 5.4 in C++14 mode:

$ g++ -std=c++14 -o x x.cpp
x.cpp: In function ‘int main()’:
x.cpp:31:6: error: could not convert ‘a’ from ‘A’ to ‘nonstd::optional_lite::optional<B>’
   f(a);
      ^

It works on gcc 8.2 in C++17 mode: https://godbolt.org/z/2XyZum

yoursunny commented 6 years ago

Replacing this line https://github.com/martinmoene/optional-lite/blob/ea0c6da30da959ef77131afa0afb0842c8e120bd/include/nonstd/optional.hpp#L714-L717 with

    template<typename U = value_type,
             typename = typename std::enable_if_t<
                          std::is_constructible<T, U&&>::value &&
                          !std::is_same<std::decay_t<U>, in_place_t>::value &&
                          !std::is_same<std::decay_t<U>, optional<T>>::value>>
    constexpr optional(U&& value)
    : has_value_( true )
    , contained( std::move( value ) )
    {}

solves the issue. Clearly this won't work in pre-C++14 compilers.

martinmoene commented 6 years ago

Note:

template<typename U = value_type, typename = typename std::enable_if_t< ... , contained( std::forward( value ) ) {}

Using std::move() breaks tests with

optional.t.cpp(720): failed: make_optional: Allows to copy-construct optional: s.state != moved_from for 9 != 9

The change should perhaps be part of a larger update to more closely follow std::optional and look like:

    template< typename U = T >
    optional_constexpr explicit optional( U && value, optional_REQUIRES(
        std::is_constructible<T, U&&>::value
        && !std::is_same<typename std20::remove_cvref<U>::type, in_place_t>::value
        && !std::is_same<typename std20::remove_cvref<U>::type, optional<T>>::value
        && !std::is_convertible<U&&, T>::value /*=> explicit */ )
    )
    : has_value_( true )
    , contained( std::forward<U>( value ) )
    {}

    template< typename U = T >
    optional_constexpr optional( U && value, optional_REQUIRES(
        std::is_constructible<T, U&&>::value
        && !std::is_same<typename std20::remove_cvref<U>::type, in_place_t>::value
        && !std::is_same<typename std20::remove_cvref<U>::type, optional<T>>::value
        && std::is_convertible<U&&, T>::value /*=> non-explicit */ )
    )
    : has_value_( true )
    , contained( std::forward<U>( value ) )
    {}
Pesa commented 6 years ago

operator=(U&&) has a similar problem: it currently requires std::is_same but it should be more permissive according to the C++17 standard.

martinmoene commented 6 years ago

Thanks. I'm reviewing constructors and assignments w.r.t. C++17/C++20.

martinmoene commented 6 years ago

... more to follow