From 8607772d4f9e21f53c1abfd9379737403b97f430 Mon Sep 17 00:00:00 2001 From: Steven Le Rouzic Date: Sun, 5 Jan 2025 15:25:45 +0100 Subject: Fix a few mistakes in option, and make it trivial when possible --- asl/box.hpp | 4 +- asl/maybe_uninit.hpp | 82 +++++++------ asl/meta.hpp | 12 +- asl/option.hpp | 243 ++++++++++++++------------------------- asl/tests/box_tests.cpp | 4 +- asl/tests/maybe_uninit_tests.cpp | 16 ++- asl/tests/option_tests.cpp | 18 +-- asl/utility.hpp | 3 + 8 files changed, 173 insertions(+), 209 deletions(-) (limited to 'asl') diff --git a/asl/box.hpp b/asl/box.hpp index 491338f..d018c03 100644 --- a/asl/box.hpp +++ b/asl/box.hpp @@ -16,7 +16,7 @@ class box ASL_NO_UNIQUE_ADDRESS Allocator m_alloc; public: - explicit constexpr box(niche) + explicit constexpr box(niche_t) requires default_constructible : m_ptr{nullptr} , m_alloc{} @@ -78,7 +78,7 @@ public: return m_ptr; } - constexpr bool operator==(niche) const + constexpr bool operator==(niche_t) const { return m_ptr == nullptr; } diff --git a/asl/maybe_uninit.hpp b/asl/maybe_uninit.hpp index e59cfe0..4f60e4d 100644 --- a/asl/maybe_uninit.hpp +++ b/asl/maybe_uninit.hpp @@ -1,60 +1,72 @@ #pragma once -#include "asl/layout.hpp" -#include "asl/memory.hpp" #include "asl/meta.hpp" #include "asl/utility.hpp" +#include "asl/memory.hpp" namespace asl { template -class maybe_uninit +union maybe_uninit { - union - { - alignas(align_of) char m_storage[size_of]; - T m_value; - }; +private: + T m_value; public: - constexpr maybe_uninit() {} // NOLINT(*-member-init) + constexpr maybe_uninit() requires trivially_default_constructible = default; + constexpr maybe_uninit() requires (!trivially_default_constructible) {} // NOLINT - maybe_uninit(const maybe_uninit&) = delete; - maybe_uninit(maybe_uninit&&) = delete; - - maybe_uninit& operator=(const maybe_uninit&) = delete; - maybe_uninit& operator=(maybe_uninit&&) = delete; + template + explicit constexpr maybe_uninit(in_place_t, Args&&... args) + requires constructible_from + : m_value{ASL_FWD(args)...} + {} - constexpr ~maybe_uninit() = default; - constexpr ~maybe_uninit() requires (!trivially_destructible) {} + constexpr maybe_uninit(const maybe_uninit&) requires trivially_copy_constructible = default; + constexpr maybe_uninit(const maybe_uninit&) requires (!trivially_copy_constructible) {} // NOLINT - constexpr void* uninit_ptr() && = delete; - constexpr const void* uninit_ptr() const& { return m_storage; } - constexpr void* uninit_ptr() & { return m_storage; } - - // @Safety Pointer must only be accessed when in initialized state. - constexpr T* init_ptr_unsafe() && = delete; - constexpr const T* init_ptr_unsafe() const& { return &m_value; } - constexpr T* init_ptr_unsafe() & { return &m_value; } - - // @Safety Reference must only be accessed when in initialized state. - constexpr T&& as_init_unsafe() && { return ASL_MOVE(m_value); } - constexpr const T& as_init_unsafe() const& { return m_value; } - constexpr T& as_init_unsafe() & { return m_value; } + constexpr maybe_uninit(maybe_uninit&&) requires trivially_move_constructible = default; + constexpr maybe_uninit(maybe_uninit&&) requires (!trivially_move_constructible) {} // NOLINT + + constexpr maybe_uninit& operator=(const maybe_uninit&) requires trivially_copy_assignable = default; + constexpr maybe_uninit& operator=(const maybe_uninit&) requires (!trivially_copy_assignable) {} + + constexpr maybe_uninit& operator=(maybe_uninit&&) requires trivially_move_assignable = default; + constexpr maybe_uninit& operator=(maybe_uninit&&) requires (!trivially_move_assignable) {} + + constexpr ~maybe_uninit() requires trivially_destructible = default; + constexpr ~maybe_uninit() requires (!trivially_destructible) {} // NOLINT - // @Safety Must be called only when in uninitialized state. + // @Safety Value must not have been initialized yet template - constexpr void init_unsafe(Args&&... args) & + constexpr void construct_unsafe(Args&&... args) + requires constructible_from { - construct_at(uninit_ptr(), ASL_FWD(args)...); + construct_at(&m_value, ASL_FWD(args)...); } - // @Safety Must be called only when in initialized state. - constexpr void uninit_unsafe() & + // @Safety Value must have been initialized + template + constexpr void assign_unsafe(U&& value) + requires assignable_from { - destroy(init_ptr_unsafe()); + m_value = ASL_FWD(value); } + + // @Safety Value must have been initialized + constexpr void destroy_unsafe() + { + if constexpr (!trivially_destructible) + { + destroy(&m_value); + } + } + + // @Safety Value must have been initialized + constexpr const T& as_init_unsafe() const& { return m_value; } + constexpr T& as_init_unsafe() & { return m_value; } + constexpr T&& as_init_unsafe() && { return ASL_MOVE(m_value); } }; } // namespace asl diff --git a/asl/meta.hpp b/asl/meta.hpp index 43bd7cc..84616f4 100644 --- a/asl/meta.hpp +++ b/asl/meta.hpp @@ -190,7 +190,7 @@ template<> struct _is_integer_helper : true_type {}; template concept is_integer = _is_integer_helper>::value; template -concept equality_comparable_with = requires (const un_cvref_t& a, const un_cvref_t& b) +concept equality_comparable_with = requires (const un_cvref_t& a, const un_cvref_t& b) { { a == b } -> same_as; { b == a } -> same_as; @@ -200,16 +200,12 @@ concept equality_comparable_with = requires (const un_cvref_t& a, const un_cv template concept equality_comparable = equality_comparable_with; -struct niche {}; +struct niche_t {}; template -concept has_niche = constructible_from && - requires (const T& value, niche n) - { - { value == n } -> same_as; - }; +concept has_niche = constructible_from && equality_comparable_with; template -concept is_niche = same_as, niche>; +concept is_niche = same_as, niche_t>; } // namespace asl diff --git a/asl/option.hpp b/asl/option.hpp index b87a5ab..8c0a573 100644 --- a/asl/option.hpp +++ b/asl/option.hpp @@ -22,34 +22,24 @@ namespace option_internal { template -concept convertible_from_option = - convertible_from&> && - convertible_from&> && - convertible_from&&> && - convertible_from&&>; +concept not_constructible_from_option = + !constructible_from&> && + !constructible_from&> && + !constructible_from&&> && + !constructible_from&&>; template -concept constructible_from_option = - constructible_from&> && - constructible_from&> && - constructible_from&&> && - constructible_from&&>; +concept not_assignable_from_option = + !assignable_from&> && + !assignable_from&> && + !assignable_from&&> && + !assignable_from&&>; -template -concept assignable_from_option = - assignable_from&> && - assignable_from&> && - assignable_from&&> && - assignable_from&&>; template -concept convertible_constructible_from_option = - convertible_from_option && constructible_from_option; - - -template -concept convertible_constructible_assignable_from_option = - convertible_constructible_from_option && assignable_from_option; +concept not_constructible_assignable_from_option = + not_constructible_from_option && + not_assignable_from_option; } // namespace option_internal @@ -65,31 +55,35 @@ class option { static constexpr bool kHasNiche = has_niche; - static constexpr bool kHasInlinePayload = default_constructible || kHasNiche; - - using Storage = select_t>; using HasValueMarker = select_t; - Storage m_payload; + maybe_uninit m_payload{}; ASL_NO_UNIQUE_ADDRESS HasValueMarker m_has_value{}; + template + friend class option; + template constexpr void construct(Args&&... args) { ASL_ASSERT(!has_value()); - if constexpr (kHasInlinePayload) + if constexpr (!kHasNiche) { - m_payload = T(ASL_FWD(args)...); + m_payload.construct_unsafe(ASL_FWD(args)...); + m_has_value = true; } else { - m_payload.init_unsafe(ASL_FWD(args)...); - } - - if constexpr (!kHasNiche) - { - m_has_value = true; + if constexpr (move_assignable) + { + m_payload.assign_unsafe(ASL_MOVE(T{ASL_FWD(args)...})); + } + else + { + m_payload.destroy_unsafe(); + m_payload.construct_unsafe(ASL_FWD(args)...); + } } } @@ -97,119 +91,81 @@ class option constexpr void assign(U&& arg) { ASL_ASSERT(has_value()); - - if constexpr (kHasInlinePayload) - { - m_payload = ASL_FWD(arg); - } - else - { - m_payload.as_init_unsafe() = ASL_FWD(arg); - } + m_payload.assign_unsafe(ASL_FWD(arg)); } public: using type = T; - constexpr option() : option(nullopt) {} - - // NOLINTNEXTLINE(*-explicit-conversions) - constexpr option(nullopt_t) requires (!kHasNiche) && trivially_default_constructible {} + constexpr option() : option{nullopt} {} // NOLINTNEXTLINE(*-explicit-conversions) - constexpr option(nullopt_t) requires (!kHasNiche) && (!trivially_default_constructible) - : m_payload{} - {} + constexpr option(nullopt_t) requires (!kHasNiche) {} // NOLINTNEXTLINE(*-explicit-conversions) - constexpr option(nullopt_t) requires kHasNiche - : m_payload{niche{}} - {} + constexpr option(nullopt_t) requires kHasNiche : m_payload{in_place, niche_t{}} {} template constexpr explicit (!convertible_from) option(U&& value) requires ( kHasNiche && - constructible_from && - !same_as, option> && - !is_niche + constructible_from && + !same_as, option> ) - : m_payload(ASL_FWD(value)) + : m_payload{in_place, ASL_FWD(value)} {} template - constexpr explicit (!convertible_from) + constexpr explicit (!convertible_from) option(U&& value) requires ( - kHasInlinePayload && !kHasNiche && - constructible_from && - !same_as, option> && - !is_niche + constructible_from && + !is_option ) - : m_payload(ASL_FWD(value)) + : m_payload{in_place, ASL_FWD(value)} , m_has_value{true} {} - - template - constexpr explicit (!convertible_from) - option(U&& value) - requires ( - !kHasInlinePayload && - constructible_from && - !same_as, option> && - !is_niche - ) - : option(nullopt) - { - construct(ASL_FWD(value)); - } - constexpr option(const option& other) - requires copy_constructible && kHasInlinePayload = default; + constexpr option(const option& other) requires trivially_copy_constructible = default; + constexpr option(const option& other) requires (!copy_constructible) = delete; - constexpr option(const option& other) - requires copy_constructible && (!kHasInlinePayload) - : option(nullopt) + constexpr option(const option& other) + requires copy_constructible && (!trivially_copy_constructible) + : option{nullopt} { if (other.has_value()) { - construct(other.value()); + construct(other.m_payload.as_init_unsafe()); } } + + constexpr option(option&& other) requires trivially_move_constructible = default; + constexpr option(option&& other) requires (!move_constructible) = delete; - constexpr option(const option& other) - requires (!copy_constructible) = delete; - - constexpr option(option&& other) - requires move_constructible && kHasInlinePayload = default; - - constexpr option(option&& other) - requires move_constructible && (!kHasInlinePayload) - : option(nullopt) + constexpr option(option&& other) + requires move_constructible && (!trivially_move_constructible) + : option{nullopt} { if (other.has_value()) { - construct(ASL_MOVE(other.value())); + construct(ASL_MOVE(other.m_payload.as_init_unsafe())); } } - constexpr option(option&& other) - requires (!move_constructible) = delete; - template constexpr explicit (!convertible_from) option(const option& other) requires ( constructible_from && - !option_internal::convertible_constructible_from_option + option_internal::not_constructible_from_option ) - : option(nullopt) + : option{nullopt} { if (other.has_value()) { - construct(other.value()); + construct(other.m_payload.as_init_unsafe()); } } @@ -218,13 +174,13 @@ public: option(option&& other) requires ( constructible_from && - !option_internal::convertible_constructible_from_option + option_internal::not_constructible_from_option ) - : option(nullopt) + : option{nullopt} { if (other.has_value()) { - construct(ASL_MOVE(other.value())); + construct(ASL_MOVE(other.m_payload.as_init_unsafe())); } } @@ -237,9 +193,9 @@ public: template constexpr option& operator=(U&& value) & requires ( - assignable_from && - constructible_from && - !same_as, option> + assignable_from && + constructible_from && + !is_option ) { if (has_value()) @@ -255,13 +211,13 @@ public: } constexpr option& operator=(const option& other) & - requires (!copy_assignable || !copy_constructible) = delete; + requires (!copy_assignable) = delete; constexpr option& operator=(const option& other) & - requires copy_assignable && copy_constructible && kHasInlinePayload = default; + requires trivially_copy_assignable = default; constexpr option& operator=(const option& other) & - requires copy_assignable && copy_constructible && (!kHasInlinePayload) + requires copy_assignable && (!trivially_copy_constructible) { if (&other == this) { return *this; } @@ -283,12 +239,15 @@ public: return *this; } - + constexpr option& operator=(option&& other) & - requires move_assignable && move_constructible && kHasInlinePayload = default; + requires (!move_assignable) = delete; constexpr option& operator=(option&& other) & - requires move_assignable && move_constructible && (!kHasInlinePayload) + requires trivially_move_assignable = default; + + constexpr option& operator=(option&& other) & + requires move_assignable && (!trivially_move_constructible) { if (&other == this) { return *this; } @@ -316,18 +275,18 @@ public: requires ( constructible_from && assignable_from && - !option_internal::convertible_constructible_assignable_from_option + option_internal::not_constructible_assignable_from_option ) { if (other.has_value()) { if (has_value()) { - assign(other.value()); + assign(other.m_payload.as_init_unsafe()); } else { - construct(other.value()); + construct(other.m_payload.as_init_unsafe()); } } else if (has_value()) @@ -341,20 +300,20 @@ public: template constexpr option& operator=(option&& other) & requires ( - constructible_from && - assignable_from && - !option_internal::convertible_constructible_assignable_from_option + constructible_from && + assignable_from && + option_internal::not_constructible_assignable_from_option ) { if (other.has_value()) { if (has_value()) { - assign(ASL_MOVE(other).value()); + assign(ASL_MOVE(other.m_payload.as_init_unsafe())); } else { - construct(ASL_MOVE(other).value()); + construct(ASL_MOVE(other.m_payload.as_init_unsafe())); } } else if (has_value()) @@ -364,8 +323,8 @@ public: return *this; } - - constexpr ~option() = default; + + constexpr ~option() requires trivially_destructible = default; constexpr ~option() requires (!trivially_destructible) { reset(); @@ -379,21 +338,18 @@ public: { if constexpr (move_assignable) { - m_payload = T(niche{}); + m_payload.assign_unsafe(ASL_MOVE(T{niche_t{}})); } else { - destroy(&m_payload); - construct_at(&m_payload, niche{}); + m_payload.destroy_unsafe(); + m_payload.construct_unsafe(niche_t{}); } } else { m_has_value = false; - if constexpr (!kHasInlinePayload) - { - m_payload.uninit_unsafe(); - } + m_payload.destroy_unsafe(); } } @@ -401,7 +357,7 @@ public: { if constexpr (kHasNiche) { - return m_payload != niche{}; + return m_payload.as_init_unsafe() != niche_t{}; } else { @@ -413,40 +369,19 @@ public: constexpr T&& value() && { ASL_ASSERT_RELEASE(has_value()); - if constexpr (kHasInlinePayload) - { - return ASL_MOVE(m_payload); - } - else - { - return ASL_MOVE(m_payload).as_init_unsafe(); - } + return ASL_MOVE(m_payload).as_init_unsafe(); } constexpr T& value() & { ASL_ASSERT_RELEASE(has_value()); - if constexpr (kHasInlinePayload) - { - return m_payload; - } - else - { - return m_payload.as_init_unsafe(); - } + return m_payload.as_init_unsafe(); } constexpr const T& value() const& { ASL_ASSERT_RELEASE(has_value()); - if constexpr (kHasInlinePayload) - { - return m_payload; - } - else - { - return m_payload.as_init_unsafe(); - } + return m_payload.as_init_unsafe(); } template diff --git a/asl/tests/box_tests.cpp b/asl/tests/box_tests.cpp index 53b6dda..0395e5f 100644 --- a/asl/tests/box_tests.cpp +++ b/asl/tests/box_tests.cpp @@ -55,6 +55,8 @@ ASL_TEST(arrow) ASL_TEST(niche) { + static_assert(sizeof(asl::box) == sizeof(asl::option>)); + asl::option> opt; ASL_TEST_EXPECT(!opt.has_value()); @@ -66,7 +68,7 @@ ASL_TEST(niche) ASL_TEST_EXPECT(!opt.has_value()); bool destroyed = false; - asl::option> opt2 = asl::make_box(&destroyed); + asl::option opt2 = asl::make_box(&destroyed); ASL_TEST_EXPECT(opt2.has_value()); ASL_TEST_EXPECT(!destroyed); diff --git a/asl/tests/maybe_uninit_tests.cpp b/asl/tests/maybe_uninit_tests.cpp index 92999a2..524a10b 100644 --- a/asl/tests/maybe_uninit_tests.cpp +++ b/asl/tests/maybe_uninit_tests.cpp @@ -5,6 +5,18 @@ static_assert(asl::layout::of() == asl::layout::of>( static_assert(asl::size_of == asl::size_of>); static_assert(asl::align_of == asl::align_of>); -static_assert(asl::trivially_destructible>); -static_assert(!asl::trivially_destructible>); +#define TEST_TYPE_PROPERTIES(PRP) \ + static_assert(asl::PRP> == asl::PRP); \ + static_assert(asl::PRP> == asl::PRP); \ + static_assert(asl::PRP> == asl::PRP); \ + static_assert(asl::PRP> == asl::PRP); \ + static_assert(asl::PRP> == asl::PRP); \ + static_assert(asl::PRP> == asl::PRP); + +TEST_TYPE_PROPERTIES(trivially_default_constructible); +TEST_TYPE_PROPERTIES(trivially_copy_constructible); +TEST_TYPE_PROPERTIES(trivially_move_constructible); +TEST_TYPE_PROPERTIES(trivially_copy_assignable); +TEST_TYPE_PROPERTIES(trivially_move_assignable); +TEST_TYPE_PROPERTIES(trivially_destructible); diff --git a/asl/tests/option_tests.cpp b/asl/tests/option_tests.cpp index 557e2fe..20ee756 100644 --- a/asl/tests/option_tests.cpp +++ b/asl/tests/option_tests.cpp @@ -14,14 +14,14 @@ struct NonZero ASL_ASSERT(x != 0); } - constexpr explicit NonZero(asl::niche) : value(0) {} + constexpr explicit NonZero(asl::niche_t) : value(0) {} - constexpr bool operator==(asl::niche) const { return value == 0; } + constexpr bool operator==(asl::niche_t) const { return value == 0; } }; static_assert(asl::has_niche); static_assert(!asl::has_niche); -static_assert(sizeof(asl::option) > sizeof(int)); +static_assert(sizeof(asl::option) >= sizeof(int)); static_assert(sizeof(asl::option) == sizeof(NonZero)); static_assert(!asl::is_option); @@ -31,22 +31,26 @@ static_assert(asl::is_option>); static_assert(asl::trivially_destructible>); static_assert(!asl::trivially_destructible>); -static_assert(asl::copy_constructible>); +static_assert(asl::trivially_copy_constructible>); +static_assert(asl::trivially_copy_constructible>); static_assert(asl::copy_constructible>); static_assert(!asl::copy_constructible>); static_assert(!asl::copy_constructible>); -static_assert(asl::move_constructible>); +static_assert(asl::trivially_move_constructible>); +static_assert(asl::trivially_move_constructible>); static_assert(asl::move_constructible>); static_assert(asl::move_constructible>); static_assert(!asl::move_constructible>); -static_assert(asl::copy_assignable>); +static_assert(asl::trivially_copy_assignable>); +static_assert(asl::trivially_copy_assignable>); static_assert(asl::copy_assignable>); static_assert(!asl::copy_assignable>); static_assert(!asl::copy_assignable>); -static_assert(asl::move_assignable>); +static_assert(asl::trivially_move_assignable>); +static_assert(asl::trivially_move_assignable>); static_assert(asl::move_assignable>); static_assert(asl::move_assignable>); static_assert(!asl::move_assignable>); diff --git a/asl/utility.hpp b/asl/utility.hpp index 69838e5..4f27017 100644 --- a/asl/utility.hpp +++ b/asl/utility.hpp @@ -11,6 +11,9 @@ namespace asl { +struct in_place_t {}; +static constexpr in_place_t in_place{}; + template constexpr void swap(T& a, T& b) { -- cgit