Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp has wrong assumption #40922

Closed
jwakely mannequin opened this issue Apr 24, 2019 · 4 comments
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@jwakely
Copy link
Mannequin

jwakely mannequin commented Apr 24, 2019

Bugzilla Link 41577
Resolution FIXED
Resolved on Apr 24, 2019 19:16
Version unspecified
OS All
CC @mclow

Extended Description

std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp

This test assumes that move construction is not constexpr, based on this old wording quoted in the test:

// constexpr optional(const optional&& rhs);
//   If is_trivially_move_constructible_v is true,
//    this constructor shall be a constexpr constructor.

That was replaced by https://wg21.link/p0602r4 and now it says:

"If is_trivially_move_constructible_v is true, this constructor is trivial."

That means the constructor might still be constexpr even if it's non-trivial.

It's not clear to me whether the constructor is required to be constexpr, but I think the test is wrong to assume the move construction can't be constexpr (the test compiles using the std::optional from libstdc++, but 'lit' expects it to be ill-formed).

@mclow
Copy link
Contributor

mclow commented Apr 24, 2019

This may be a bug in our optional implementation.

Given:

#include
#include <type_traits>
#include

struct S {
constexpr S() : v_(0) {}
constexpr S(int v) : v_(v) {}
constexpr S(const S &rhs) : v_(rhs.v_) {} // not trivially copyable
constexpr S( S &&rhs) : v_(rhs.v_) {} // not trivially moveable
int v_;
};

int main(int, char**)
{
static_assert (!std::is_trivially_move_constructible_v, "");
constexpr S s1(3);
constexpr S s2 = std::move(s0);
(void) s2;

constexpr std::optional<S> o1;
constexpr std::optional<S> o2 = std::move(o1);  // not constexpr

return 0;
}

I don't see why the line marked "not constexpr" shouldn't be constexpr.

@mclow
Copy link
Contributor

mclow commented Apr 24, 2019

Better test:

#include
#include <type_traits>
#include

struct S {
constexpr S() : v_(0) {}
constexpr S(int v) : v_(v) {}
constexpr S(const S &rhs) : v_(rhs.v_) {} // not trivially copyable
constexpr S( S &&rhs) : v_(rhs.v_) {} // not trivially moveable
int v_;
};

constexpr bool test()
{
std::optional o1{3};
std::optional o2 = std::move(o1); // not constexpr
return o2.has_value();
}

int main(int, char**)
{
static_assert(test(), "");
return 0;
}

fails to compile thus:

$ totclang2a junk.cpp
junk.cpp:14:16: error: constexpr function never produces a constant expression
[-Winvalid-constexpr]
constexpr bool test()
^
junk.cpp:17:27: note: non-constexpr constructor 'optional' cannot be used in a
constant expression
std::optional o2 = std::move(o1); // not constexpr
^
/Sources/LLVM/llvm-project/libcxx/include/optional:689:41: note: declared here
_LIBCPP_INLINE_VISIBILITY constexpr optional(optional&&) = default;
^
junk.cpp:24:19: error: static_assert expression is not an integral constant
expression
static_assert(test(), "");
^~~~~~
junk.cpp:17:27: note: non-constexpr constructor 'optional' cannot be used in a
constant expression
std::optional o2 = std::move(o1); // not constexpr
^
junk.cpp:24:19: note: in call to 'test()'
static_assert(test(), "");
^
/Sources/LLVM/llvm-project/libcxx/include/optional:689:41: note: declared here
_LIBCPP_INLINE_VISIBILITY constexpr optional(optional&&) = default;
^
2 errors generated.

@mclow
Copy link
Contributor

mclow commented Apr 25, 2019

Eric pointed out that you can't have a constexpr move-assignment operator for the non-trivial case, because it has to do a placement new (unless the optional that you are constructing from is empty)

That's why my second test fails.
Apparently libstdc++ is happy with this:

constexpr bool test()
{
std::optional o1; // empty
std::optional o2 = std::move(o1);
return o2.has_value();
}

but libc++ is not.

@mclow
Copy link
Contributor

mclow commented Apr 25, 2019

Test fixed in revision 359162 - now the test is move-constructing a non-empty optional.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

No branches or pull requests

1 participant