[libc++] Implement P2988R12: std::optional<T&>
#155202
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
the
libc++
Member
|
@llvm/pr-subscribers-libcxx Author: William Tran-Viet (smallp-o-p) ChangesResolves #148131
Patch is 44.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/155202.diff 29 Files Affected:
diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst
index 358889d8dbc37..c10fc4c365407 100644
--- a/libcxx/docs/FeatureTestMacroTable.rst
+++ b/libcxx/docs/FeatureTestMacroTable.rst
@@ -480,6 +480,8 @@ Status
---------------------------------------------------------- -----------------
``__cpp_lib_not_fn`` ``202306L``
---------------------------------------------------------- -----------------
+ ``__cpp_lib_optional`` ``202506L``
+ ---------------------------------------------------------- -----------------
``__cpp_lib_optional_range_support`` ``202406L``
---------------------------------------------------------- -----------------
``__cpp_lib_out_ptr`` ``202311L``
diff --git a/libcxx/docs/ReleaseNotes/22.rst b/libcxx/docs/ReleaseNotes/22.rst
index a26c5476d421b..e070169c87ba3 100644
--- a/libcxx/docs/ReleaseNotes/22.rst
+++ b/libcxx/docs/ReleaseNotes/22.rst
@@ -39,6 +39,7 @@ Implemented Papers
------------------
- P2321R2: ``zip`` (`Github <https://github.com/llvm/llvm-project/issues/105169>`__) (The paper is partially implemented. ``zip_transform_view`` is implemented in this release)
+- P2988R12: ``std::optional<T&>`` (`Github <https://github.com/llvm/llvm-project/issues/148131>`__)
- P3168R2: Give ``std::optional`` Range Support (`Github <https://github.com/llvm/llvm-project/issues/105430>`__)
Improvements and New Features
diff --git a/libcxx/docs/Status/Cxx2cPapers.csv b/libcxx/docs/Status/Cxx2cPapers.csv
index 3b8b2b7ad0b3f..5e28875c26944 100644
--- a/libcxx/docs/Status/Cxx2cPapers.csv
+++ b/libcxx/docs/Status/Cxx2cPapers.csv
@@ -122,7 +122,7 @@
"`P3293R3 <https://wg21.link/P3293R3>`__","Splicing a base class subobject","2025-06 (Sofia)","","",""
"`P3491R3 <https://wg21.link/P3491R3>`__","``define_static_{string,object,array}``","2025-06 (Sofia)","","",""
"`P3096R12 <https://wg21.link/P3096R12>`__","Function Parameter Reflection in Reflection for C++26","2025-06 (Sofia)","","",""
-"`P2988R12 <https://wg21.link/P2988R12>`__","``std::optional<T&>``","2025-06 (Sofia)","","",""
+"`P2988R12 <https://wg21.link/P2988R12>`__","``std::optional<T&>``","2025-06 (Sofia)","|Complete|","22",""
"`P3348R4 <https://wg21.link/P3348R4>`__","C++26 should refer to C23 not C17","2025-06 (Sofia)","","",""
"`P3037R6 <https://wg21.link/P3037R6>`__","``constexpr`` ``std::shared_ptr`` and friends","2025-06 (Sofia)","","",""
"`P3284R4 <https://wg21.link/P3284R4>`__","``write_env`` and ``unstoppable`` Sender Adaptors","2025-06 (Sofia)","","",""
diff --git a/libcxx/include/__iterator/wrap_iter.h b/libcxx/include/__iterator/wrap_iter.h
index 7610586ddecbb..8e5c7388812d3 100644
--- a/libcxx/include/__iterator/wrap_iter.h
+++ b/libcxx/include/__iterator/wrap_iter.h
@@ -117,8 +117,8 @@ class __wrap_iter {
friend class span;
template <class _Tp, size_t _Size>
friend struct array;
- template <class _Tp>
- friend class optional;
+ template <class _Tp, class>
+ friend struct __optional_iterator;
};
template <class _Iter1>
diff --git a/libcxx/include/optional b/libcxx/include/optional
index 39fcaa2c2ec18..489691f33e0cd 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -161,8 +161,8 @@ namespace std {
constexpr T &value() &;
constexpr T &&value() &&;
constexpr const T &&value() const &&;
- template<class U> constexpr T value_or(U &&) const &;
- template<class U> constexpr T value_or(U &&) &&;
+ template<class U = remove_cv_t<T>> constexpr T value_or(U&& v) const &;
+ template<class U = remove_cv_t<T>> constexpr T value_or(U&& v) &&;
// [optional.monadic], monadic operations
template<class F> constexpr auto and_then(F&& f) &; // since C++23
@@ -412,9 +412,6 @@ struct __optional_storage_base : __optional_destruct_base<_Tp> {
}
};
-// optional<T&> is currently required to be ill-formed. However, it may
-// be allowed in the future. For this reason, it has already been implemented
-// to ensure we can make the change in an ABI-compatible manner.
template <class _Tp>
struct __optional_storage_base<_Tp, true> {
using value_type = _Tp;
@@ -607,19 +604,19 @@ struct __is_std_optional : false_type {};
template <class _Tp>
struct __is_std_optional<optional<_Tp>> : true_type {};
-template <class _Tp>
-class _LIBCPP_DECLSPEC_EMPTY_BASES optional
- : private __optional_move_assign_base<_Tp>,
- private __optional_sfinae_ctor_base_t<_Tp>,
- private __optional_sfinae_assign_base_t<_Tp> {
- using __base _LIBCPP_NODEBUG = __optional_move_assign_base<_Tp>;
+template <class _Tp, class = void>
+struct __optional_iterator {};
- using __pointer _LIBCPP_NODEBUG = std::add_pointer_t<_Tp>;
- using __const_pointer _LIBCPP_NODEBUG = std::add_pointer_t<const _Tp>;
+template <class _Tp>
+struct __optional_iterator<
+ _Tp,
+ enable_if_t<!(is_lvalue_reference_v<_Tp> && is_function_v<__libcpp_remove_reference_t<_Tp>>) &&
+ !(is_lvalue_reference_v<_Tp> && is_array_v<__libcpp_remove_reference_t<_Tp>>)> > {
+private:
+ using __pointer _LIBCPP_NODEBUG = std::add_pointer_t<remove_reference_t<_Tp>>;
+ using __const_pointer _LIBCPP_NODEBUG = std::add_pointer_t<const remove_reference_t<_Tp>>;
public:
- using value_type = _Tp;
-
# if _LIBCPP_STD_VER >= 26
# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
using iterator = __bounded_iter<__wrap_iter<__pointer>>;
@@ -628,18 +625,67 @@ public:
using iterator = __wrap_iter<__pointer>;
using const_iterator = __wrap_iter<__const_pointer>;
# endif
+
+ // [optional.iterators], iterator support
+ _LIBCPP_HIDE_FROM_ABI constexpr iterator begin() noexcept {
+ auto& __derived_self = static_cast<optional<_Tp>&>(*this);
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
+ return std::__make_bounded_iter(
+ std::__wrap_iter<__pointer>(std::addressof(__derived_self.__get())),
+ std::__wrap_iter<__pointer>(std::addressof(__derived_self.__get())),
+ std::__wrap_iter<__pointer>(std::addressof(__derived_self.__get()) + (__derived_self.has_value() ? 1 : 0)));
+# else
+ return iterator(std::addressof(__derived_self.__get()));
+# endif
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr const_iterator begin() const noexcept {
+ auto& __derived_self = static_cast<const optional<_Tp>&>(*this);
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
+ return std::__make_bounded_iter(
+ std::__wrap_iter<__const_pointer>(std::addressof(__derived_self.__get())),
+ std::__wrap_iter<__const_pointer>(std::addressof(__derived_self.__get())),
+ std::__wrap_iter<__const_pointer>(
+ std::addressof(__derived_self.__get()) + (__derived_self.has_value() ? 1 : 0)));
+# else
+ return const_iterator(std::addressof(__derived_self.__get()));
+# endif
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr iterator end() noexcept {
+ return begin() + (static_cast<optional<_Tp>&>(*this).has_value() ? 1 : 0);
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr const_iterator end() const noexcept {
+ return begin() + (static_cast<const optional<_Tp>&>(*this).has_value() ? 1 : 0);
+ }
# endif
+};
+
+template <class _Tp>
+class _LIBCPP_DECLSPEC_EMPTY_BASES optional
+ : private __optional_move_assign_base<_Tp>,
+ private __optional_sfinae_ctor_base_t<_Tp>,
+ private __optional_sfinae_assign_base_t<_Tp>,
+ public __optional_iterator<_Tp> {
+ using __base _LIBCPP_NODEBUG = __optional_move_assign_base<_Tp>;
+
+public:
+ using value_type = _Tp;
+
using __trivially_relocatable _LIBCPP_NODEBUG =
conditional_t<__libcpp_is_trivially_relocatable<_Tp>::value, optional, void>;
using __replaceable _LIBCPP_NODEBUG = conditional_t<__is_replaceable_v<_Tp>, optional, void>;
private:
- // Disable the reference extension using this static assert.
static_assert(!is_same_v<__remove_cvref_t<value_type>, in_place_t>,
"instantiation of optional with in_place_t is ill-formed");
static_assert(!is_same_v<__remove_cvref_t<value_type>, nullopt_t>,
"instantiation of optional with nullopt_t is ill-formed");
- static_assert(!is_reference_v<value_type>, "instantiation of optional with a reference type is ill-formed");
+# if _LIBCPP_STD_VER >= 26
+ static_assert(!is_rvalue_reference_v<_Tp>, "instantiation of optional with an rvalue reference type is ill-formed");
+# else
+ static_assert(!is_reference_v<_Tp>, "instantiation of optional with a reference type is ill-formed");
+# endif
static_assert(is_destructible_v<value_type>, "instantiation of optional with a non-destructible type is ill-formed");
static_assert(!is_array_v<value_type>, "instantiation of optional with an array type is ill-formed");
@@ -832,34 +878,6 @@ public:
}
}
-# if _LIBCPP_STD_VER >= 26
- // [optional.iterators], iterator support
- _LIBCPP_HIDE_FROM_ABI constexpr iterator begin() noexcept {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
- return std::__make_bounded_iter(
- std::__wrap_iter<__pointer>(std::addressof(this->__get())),
- std::__wrap_iter<__pointer>(std::addressof(this->__get())),
- std::__wrap_iter<__pointer>(std::addressof(this->__get()) + (this->has_value() ? 1 : 0)));
-# else
- return iterator(std::addressof(this->__get()));
-# endif
- }
-
- _LIBCPP_HIDE_FROM_ABI constexpr const_iterator begin() const noexcept {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
- return std::__make_bounded_iter(
- std::__wrap_iter<__const_pointer>(std::addressof(this->__get())),
- std::__wrap_iter<__const_pointer>(std::addressof(this->__get())),
- std::__wrap_iter<__const_pointer>(std::addressof(this->__get()) + (this->has_value() ? 1 : 0)));
-# else
- return const_iterator(std::addressof(this->__get()));
-# endif
- }
-
- _LIBCPP_HIDE_FROM_ABI constexpr iterator end() noexcept { return begin() + (this->has_value() ? 1 : 0); }
- _LIBCPP_HIDE_FROM_ABI constexpr const_iterator end() const noexcept { return begin() + (this->has_value() ? 1 : 0); }
-# endif
-
_LIBCPP_HIDE_FROM_ABI constexpr add_pointer_t<value_type const> operator->() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(this->has_value(), "optional operator-> called on a disengaged value");
return std::addressof(this->__get());
@@ -919,20 +937,38 @@ public:
return std::move(this->__get());
}
- template <class _Up>
+ template <class _Up = remove_cv_t<_Tp>>
+# if _LIBCPP_STD_VER >= 26
+ requires(!(is_lvalue_reference_v<_Tp> && is_function_v<__libcpp_remove_reference_t<_Tp>>) &&
+ !(is_lvalue_reference_v<_Tp> && is_array_v<__libcpp_remove_reference_t<_Tp>>))
+# endif
_LIBCPP_HIDE_FROM_ABI constexpr value_type value_or(_Up&& __v) const& {
static_assert(is_copy_constructible_v<value_type>, "optional<T>::value_or: T must be copy constructible");
static_assert(is_convertible_v<_Up, value_type>, "optional<T>::value_or: U must be convertible to T");
return this->has_value() ? this->__get() : static_cast<value_type>(std::forward<_Up>(__v));
}
- template <class _Up>
+ template <class _Up = remove_cv_t<_Tp>>
+# if _LIBCPP_STD_VER >= 26
+ requires(!is_lvalue_reference_v<_Tp>)
+# endif
_LIBCPP_HIDE_FROM_ABI constexpr value_type value_or(_Up&& __v) && {
static_assert(is_move_constructible_v<value_type>, "optional<T>::value_or: T must be move constructible");
static_assert(is_convertible_v<_Up, value_type>, "optional<T>::value_or: U must be convertible to T");
return this->has_value() ? std::move(this->__get()) : static_cast<value_type>(std::forward<_Up>(__v));
}
+# if _LIBCPP_STD_VER >= 26
+ template <class _Up = remove_cv_t<_Tp>>
+ requires(is_lvalue_reference_v<_Tp> &&
+ !(is_function_v<__libcpp_remove_reference_t<_Tp>> || is_array_v<__libcpp_remove_reference_t<_Tp>>))
+ _LIBCPP_HIDE_FROM_ABI constexpr value_type value_or(_Up&& __v) && {
+ static_assert(is_move_constructible_v<value_type>, "optional<T>::value_or: T must be move constructible");
+ static_assert(is_convertible_v<_Up, value_type>, "optional<T>::value_or: U must be convertible to T");
+ return this->has_value() ? this->__get() : static_cast<value_type>(std::forward<_Up>(__v));
+ }
+# endif
+
# if _LIBCPP_STD_VER >= 23
template <class _Func>
_LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) & {
diff --git a/libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp b/libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp
index 3cdd7553e2e5d..472049a91a8d1 100644
--- a/libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp
+++ b/libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp
@@ -23,8 +23,7 @@ concept has_iterator_aliases = requires {
static_assert(has_iterator_aliases<std::optional<int>>);
static_assert(has_iterator_aliases<std::optional<const int>>);
-
-// TODO: Uncomment these once P2988R12 is implemented, as they would be testing optional<T&>
-
-// static_assert(!has_iterator_aliases<std::optional<int (&)[]>>);
-// static_assert(!has_iterator_aliases<std::optional<void (&)(int, char)>>);
+static_assert(has_iterator_aliases<std::optional<int&>>);
+static_assert(has_iterator_aliases<std::optional<const int&>>);
+static_assert(!has_iterator_aliases<std::optional<int(&)[1]>>);
+static_assert(!has_iterator_aliases<std::optional<int(&)()>>);
diff --git a/libcxx/test/libcxx/utilities/optional/optional.object/optional.object.observe/value_or.compile.pass.cpp b/libcxx/test/libcxx/utilities/optional/optional.object/optional.object.observe/value_or.compile.pass.cpp
new file mode 100644
index 0000000000000..298e2f6f55d4d
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/optional/optional.object/optional.object.observe/value_or.compile.pass.cpp
@@ -0,0 +1,27 @@
+
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: std-at-least-c++26
+
+// <optional>
+
+// template <class U> T optional<T>::value_or(U&&);
+
+#include <optional>
+
+template <typename Opt, typename T>
+concept has_value_or = requires(Opt opt, T&& t) {
+ { opt.value_or(t) } -> std::same_as<T>;
+};
+
+static_assert(has_value_or<std::optional<int>, int>);
+static_assert(has_value_or<std::optional<int&>, int&>);
+static_assert(has_value_or<std::optional<const int&>, const int&>);
+static_assert(!has_value_or<std::optional<int(&)[1]>&&, int(&)[1]>);
+static_assert(!has_value_or<std::optional<int(&)()>&&, int(&)()>);
diff --git a/libcxx/test/std/utilities/optional/optional.iterator/begin.pass.cpp b/libcxx/test/std/utilities/optional/optional.iterator/begin.pass.cpp
index df95a8df3793f..81234525923a1 100644
--- a/libcxx/test/std/utilities/optional/optional.iterator/begin.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.iterator/begin.pass.cpp
@@ -21,7 +21,8 @@
template <typename T>
constexpr bool test() {
- std::optional<T> opt{T{}};
+ std::remove_reference_t<T> t = std::remove_reference_t<T>{};
+ std::optional<T> opt{t};
{ // begin() is marked noexcept
static_assert(noexcept(opt.begin()));
@@ -53,6 +54,10 @@ constexpr bool tests() {
assert(test<char>());
assert(test<const int>());
assert(test<const char>());
+ assert(test<int&>());
+ assert(test<char&>());
+ assert(test<const int&>());
+ assert(test<const char&>());
return true;
}
diff --git a/libcxx/test/std/utilities/optional/optional.iterator/end.pass.cpp b/libcxx/test/std/utilities/optional/optional.iterator/end.pass.cpp
index 966c3e7441880..c62c9fc7746d6 100644
--- a/libcxx/test/std/utilities/optional/optional.iterator/end.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.iterator/end.pass.cpp
@@ -17,6 +17,7 @@
#include <iterator>
#include <optional>
#include <ranges>
+#include <type_traits>
#include <utility>
template <typename T>
@@ -41,7 +42,8 @@ constexpr bool test() {
assert(it2 == std::as_const(disengaged).end());
}
- std::optional<T> engaged{T{}};
+ std::remove_reference_t<T> t = std::remove_reference_t<T>{};
+ std::optional<T> engaged{t};
{ // end() != begin() if the optional is engaged
auto it = engaged.end();
@@ -62,6 +64,10 @@ constexpr bool tests() {
assert(test<char>());
assert(test<const int>());
assert(test<const char>());
+ assert(test<int&>());
+ assert(test<char&>());
+ assert(test<const int&>());
+ assert(test<const char&>());
return true;
}
diff --git a/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp b/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp
index 1203290a0290a..34d0d6b135564 100644
--- a/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp
@@ -20,9 +20,10 @@
#include <type_traits>
#include <utility>
-template <typename T, T __val>
+template <typename T, std::remove_reference_t<T> __val>
constexpr bool test() {
- std::optional<T> opt{__val};
+ std::remove_reference_t<T> v{__val};
+ std::optional<T> opt{v};
{ // Dereferencing an iterator of an engaged optional will return the same value that the optional holds.
auto it = opt.begin();
@@ -41,13 +42,14 @@ constexpr bool test() {
assert(std::random_access_iterator<decltype(it2)>);
}
- { // const_iterator::value_type == std::remove_cv_t<T>, const_iterator::reference == const T&, iterator::value_type = std::remove_cv_t<T>, iterator::reference == T&
+ { // const_iterator::value_type == std::remove_cvref_t<T>, const_iterator::reference == const T&, iterator::value_type = std::remove_cvref_t<T>, iterator::reference == T&
+ // std::remove_cv_t is impossible for optional<T&>
auto it = opt.begin();
auto it2 = std::as_const(opt).begin();
- assert((std::is_same_v<typename decltype(it)::value_type, std::remove_cv_t<T>>));
- assert((std::is_same_v<typename decltype(it)::reference, T&>));
- assert((std::is_same_v<typename decltype(it2)::value_type, std::remove_cv_t<T>>));
- assert((std::is_same_v<typename decltype(it2)::reference, const T&>));
+ assert((std::is_same_v<typename decltype(it)::value_type, std::remove_cvref_t<T>>));
+ assert((std::is_same_v<typename decltype(it)::reference, std::remove_reference_t<T>&>));
+ assert((std::is_same_v<typename decltype(it2)::value_type, std::remove_cvref_t<T>>));
+ assert((std::is_same_v<typename decltype(it2)::reference, const std::remove_reference_t<T>&>));
}
{ // std::ranges::size for an engaged optional<T> == 1, disengaged optional<T> == 0
@@ -68,13 +70,13 @@ constexpr bool test() {
// An optional with value that is reset will have a begin() == end(), then when it is reassigned a value,
// begin() != end(), and *begin() will contain the new value.
{
- std::optional<T> val{__val};
+ std::optional<T> val{v};
assert(val.begin() != val.end());
val.reset();
assert(val.begin() == val.end());
- val.emplace(__val);
+ val.emplace(v);
assert(val.begin() != val.end());
- assert(*(val.begin()) == __val);
+ assert(*(val.begin()) == v);
}
return true;
@@ -86,6 +88,11 @@ constexpr bool tests() {
assert((test<bool, true>()));
assert((test<const int, 2>()));
assert((test<const char, 'b'>()));
+ assert((test<int&, 1>()));
+ assert((test<char&, 'a'>()));
+ assert((test<bool&, true>()));
+ assert((test<const int&, 2>()));
+ assert((test<const char&, 'b'>()));
return true;
}
diff --git a/libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp b/libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp
index 97305d976e066..3d945e437151e 100644
--- a/libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp
@@ -257,8 +257,88 @@ c...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
H-G-Hristov
reviewed
Aug 25, 2025
libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp
Outdated
Show resolved
Hide resolved
smallp-o-p
changed the title
std::optional<T&>
smallp-o-p
force-pushed
the
implement-P2988R12
branch
2 times, most recently
from
a5e5e09 to
c12481f
Compare
Contributor
Author
|
Marking as ready + a reminder ping |
Member
|
afaict, the physical layout representation of a an struct OptionalFooRef {
union {
char __null_state_;
Foo& __val_;
};
bool __engaged_;
};I think a more optimal representation would be struct OptionalFooRef {
// nullopt encoded as nullptr, since `nullptr` is not a valid reference
Foo* __val_;
};This would allow me to use Given that (Please note that I am not a contributor / reviewer for libc++. So please wait for feedback from others before acting on my comment. Not sure if I might be sending you into the wrong direction here 🙂) |
Contributor
Author
The storage base for Either way you'll be pleased to know the original implementers thought of this already 😄 |
Contributor
Such a representation has been already implemented for years. llvm-project/libcxx/include/optional Lines 415 to 485 in 8553bd2 @smallp-o-p However, I'm not sure whether |
frederick-vs-ja
requested changes
Sep 26, 2025
Contributor
Author
|
Wonder what's up with the FreeBSD bot, seems like it can't find |
Contributor
It's on Clang 19. It should be updated fairly soon. |
Contributor
Author
|
Reminder ping |
Contributor
frederick-vs-ja
left a comment
frederick-vs-ja
left a comment
There was a problem hiding this comment.
Looks mostly fine to me. Could you update the branch (together with addressing the review comments) to see whether CI passes?
...ities/optional/optional.object/optional.object.ctor/ref_constructs_from_temporary.verify.cpp
Outdated
Show resolved
Hide resolved
...test/std/utilities/optional/optional.object/optional_requires_destructible_object.verify.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ref_t.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.object/optional.object.observe/has_value.pass.cpp
Outdated
Show resolved
Hide resolved
smallp-o-p
force-pushed
the
implement-P2988R12
branch
from
2459f20 to
aec5cce
Compare
frederick-vs-ja
approved these changes
Oct 26, 2025
Contributor
frederick-vs-ja
left a comment
frederick-vs-ja
left a comment
There was a problem hiding this comment.
LGTM modulo nit.
libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
Outdated
Show resolved
Hide resolved
frederick-vs-ja
requested changes
Oct 26, 2025
Contributor
There was a problem hiding this comment.
Ah. I was too rushed to approve this. You seemed to miss modifications of swap functions (both member and non-member ones) and make_optional.
We should verify
- that
optional<T&>::swapis alwaysnoexceptand only swap the internal pointers, and - that
std::swapforoptional<T&>behaves same asoptional<T&>::swap, and - the constraints of the
std::swap, and - modification to
std::make_optional, especially thatstd::make_optional<X&>(x)can call different overloads until and since C++26.
Contributor
Author
Just to confirm, is the intended change to disallow |
frederick-vs-ja
approved these changes
Nov 1, 2025
Contributor
Author
|
Reminder ping for more reviews 😄 |
Contributor
frederick-vs-ja
left a comment
frederick-vs-ja
left a comment
There was a problem hiding this comment.
I've just come up with some comments. Not sure whether any of them is blocking.
| // static_assert(!has_iterator_aliases<std::optional<void (&)(int, char)>>); | ||
| static_assert(has_iterator_aliases<std::optional<int&>>); | ||
| static_assert(has_iterator_aliases<std::optional<const int&>>); | ||
| static_assert(!has_iterator_aliases<std::optional<int (&)[1]>>); |
Contributor
There was a problem hiding this comment.
Looks like that we should achieve the following results, per the current resolution of LWG4308.
Suggested change
| static_assert(!has_iterator_aliases<std::optional<int (&)[1]>>); | |
| static_assert(has_iterator_aliases<std::optional<int (&)[1]>>); | |
| static_assert(!has_iterator_aliases<std::optional<int (&)[]>>); |
The product code needs to be correspondingly adjusted. We can do this in a following-up PR as LWG4308 is not formally accepted yet (but it will be soon).
| assert(test<int&>()); | ||
| assert(test<char&>()); | ||
| assert(test<const int&>()); | ||
| assert(test<const char&>()); |
Contributor
There was a problem hiding this comment.
Ditto, if you think it's better not to delay implementing LWG4308, cases for int (&)[42] and its friends should also be added.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Author
I believe |
libcxx/test/std/utilities/optional/optional.iterator/borrowed_range.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.iterator/borrowed_range.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.iterator/borrowed_range.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.iterator/borrowed_range.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.iterator/borrowed_range.compile.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/utilities/optional/optional.iterator/borrowed_range.compile.pass.cpp
Outdated
Show resolved
Hide resolved
Contributor
|
Merging now. I believe this also implements P3836R2 "Make |
Contributor
@smallp-o-p @frederick-vs-ja Thanks! Congratulations! Can't wait to use it. Hope to see that in MS STL soon too! |
Contributor
|
@smallp-o-p To complete the implementation: would you mind revisiting |
Contributor
This is seems almost orthogonal to this PR. I think it would be better to open a series of task issues for |
Contributor
Please don't. we can have a single issue about that, but one for each header is really not necessary. I'd also like to note that IMO patches implementing new stuff should add |
philnik777
reviewed
Nov 12, 2025
Comment on lines
+1437
to
+1445
| struct __make_optional_barrier_tag { | ||
| explicit __make_optional_barrier_tag() = default; | ||
| }; | ||
|
|
||
| template < | ||
| # if _LIBCPP_STD_VER >= 26 | ||
| __make_optional_barrier_tag = __make_optional_barrier_tag{}, | ||
| # endif | ||
| class _Tp> |
Contributor
Author
There was a problem hiding this comment.
Or if you were wondering why specifically it's a __make_optional_barrier_tag struct, I was just following what variant does to do the same thing.
Contributor
A GitHub issue could be helpful to coordinate the effort if we would like to have one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #148131
std::optional<T&>implementationoptional<T(&)(...)>andoptional<T(&)[]>but disablesvalue_or()andoptional::iterator+ alliteratorrelated functions