[libc++] Implement P3168R2: Give optional range support #149441
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: William Tran-Viet (smallp-o-p) ChangesResolves #105430
Patch is 20.15 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/149441.diff 12 Files Affected:
diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst
index 61805726a4ff0..5114ae59d7c67 100644
--- a/libcxx/docs/FeatureTestMacroTable.rst
+++ b/libcxx/docs/FeatureTestMacroTable.rst
@@ -480,7 +480,7 @@ Status
---------------------------------------------------------- -----------------
``__cpp_lib_not_fn`` ``202306L``
---------------------------------------------------------- -----------------
- ``__cpp_lib_optional_range_support`` *unimplemented*
+ ``__cpp_lib_optional_range_support`` ``202406L``
---------------------------------------------------------- -----------------
``__cpp_lib_out_ptr`` ``202311L``
---------------------------------------------------------- -----------------
diff --git a/libcxx/docs/ReleaseNotes/21.rst b/libcxx/docs/ReleaseNotes/21.rst
index 6f18b61284f49..b8fd5e7221251 100644
--- a/libcxx/docs/ReleaseNotes/21.rst
+++ b/libcxx/docs/ReleaseNotes/21.rst
@@ -53,7 +53,7 @@ Implemented Papers
- P2711R1: Making multi-param constructors of ``views`` ``explicit`` (`Github <https://github.com/llvm/llvm-project/issues/105252>`__)
- P2770R0: Stashing stashing ``iterators`` for proper flattening (`Github <https://github.com/llvm/llvm-project/issues/105250>`__)
- P2655R3: ``common_reference_t`` of ``reference_wrapper`` Should Be a Reference Type (`Github <https://github.com/llvm/llvm-project/issues/105260>`__)
-
+- 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 febb0c176f9c4..95f0d08fdde1c 100644
--- a/libcxx/docs/Status/Cxx2cPapers.csv
+++ b/libcxx/docs/Status/Cxx2cPapers.csv
@@ -66,7 +66,7 @@
"`P2747R2 <https://wg21.link/P2747R2>`__","``constexpr`` placement new","2024-06 (St. Louis)","|Complete|","20",""
"`P2997R1 <https://wg21.link/P2997R1>`__","Removing the common reference requirement from the indirectly invocable concepts","2024-06 (St. Louis)","|Complete|","19","Implemented as a DR against C++20. (MSVC STL and libstdc++ will do the same.)"
"`P2389R2 <https://wg21.link/P2389R2>`__","``dextents`` Index Type Parameter","2024-06 (St. Louis)","|Complete|","19",""
-"`P3168R2 <https://wg21.link/P3168R2>`__","Give ``std::optional`` Range Support","2024-06 (St. Louis)","","",""
+"`P3168R2 <https://wg21.link/P3168R2>`__","Give ``std::optional`` Range Support","2024-06 (St. Louis)","","21","|Complete|"
"`P3217R0 <https://wg21.link/P3217R0>`__","Adjoints to 'Enabling list-initialization for algorithms': find_last","2024-06 (St. Louis)","","",""
"`P2985R0 <https://wg21.link/P2985R0>`__","A type trait for detecting virtual base classes","2024-06 (St. Louis)","|Complete|","20",""
"`P0843R14 <https://wg21.link/P0843R14>`__","``inplace_vector``","2024-06 (St. Louis)","","",""
diff --git a/libcxx/include/__iterator/wrap_iter.h b/libcxx/include/__iterator/wrap_iter.h
index 2b5bc489dd44c..7610586ddecbb 100644
--- a/libcxx/include/__iterator/wrap_iter.h
+++ b/libcxx/include/__iterator/wrap_iter.h
@@ -117,6 +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 _Iter1>
diff --git a/libcxx/include/optional b/libcxx/include/optional
index e81bff50daad6..1292d969fc9ed 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -20,6 +20,11 @@ namespace std {
template <class T>
class optional;
+ template<class T>
+ constexpr bool ranges::enable_view<optional<T>> = true;
+ template<class T>
+ constexpr auto format_kind<optional<T>> = range_format::disabled;
+
template<class T>
concept is-derived-from-optional = requires(const T& t) { // exposition only
[]<class U>(const optional<U>&){ }(t);
@@ -102,6 +107,8 @@ namespace std {
class optional {
public:
using value_type = T;
+ using iterator = implementation-defined; // see [optional.iterators]
+ using const_iterator = implementation-defined; // see [optional.iterators]
// [optional.ctor], constructors
constexpr optional() noexcept;
@@ -135,6 +142,12 @@ namespace std {
// [optional.swap], swap
void swap(optional &) noexcept(see below ); // constexpr in C++20
+ // [optional.iterators], iterator support
+ constexpr iterator begin() noexcept;
+ constexpr const_iterator begin() const noexcept;
+ constexpr iterator end() noexcept;
+ constexpr const_iterator end() const noexcept;
+
// [optional.observe], observers
constexpr T const *operator->() const noexcept;
constexpr T *operator->() noexcept;
@@ -233,6 +246,13 @@ namespace std {
# include <initializer_list>
# include <version>
+# if _LIBCPP_STD_VER >= 26
+# include <__cstddef/ptrdiff_t.h>
+# include <__format/range_format.h>
+# include <__iterator/wrap_iter.h>
+# include <__ranges/enable_view.h>
+# endif
+
// standard-mandated includes
// [optional.syn]
@@ -567,6 +587,14 @@ using __optional_sfinae_assign_base_t _LIBCPP_NODEBUG =
template <class _Tp>
class optional;
+# if _LIBCPP_STD_VER >= 26
+template <class _Tp>
+constexpr bool ranges::enable_view<optional<_Tp>> = true;
+
+template <class _Tp>
+constexpr range_format format_kind<optional<_Tp>> = range_format::disabled;
+# endif
+
# if _LIBCPP_STD_VER >= 20
template <class _Tp>
@@ -586,8 +614,17 @@ class _LIBCPP_DECLSPEC_EMPTY_BASES optional
private __optional_sfinae_assign_base_t<_Tp> {
using __base _LIBCPP_NODEBUG = __optional_move_assign_base<_Tp>;
+# if _LIBCPP_STD_VER >= 26
+ using pointer = std::add_pointer_t<std::remove_cvref_t<_Tp>>;
+ using const_pointer = std::add_pointer_t<const std::remove_cvref_t<_Tp>>;
+# endif
+
public:
using value_type = _Tp;
+# if _LIBCPP_STD_VER >= 26
+ using iterator = __wrap_iter<pointer>;
+ using const_iterator = __wrap_iter<const_pointer>;
+# endif
using __trivially_relocatable _LIBCPP_NODEBUG =
conditional_t<__libcpp_is_trivially_relocatable<_Tp>::value, optional, void>;
@@ -792,6 +829,18 @@ public:
}
}
+# if _LIBCPP_STD_VER >= 26
+ // [optional.iterators], iterator support
+ _LIBCPP_HIDE_FROM_ABI constexpr iterator begin() noexcept { return iterator(std::addressof(this->__get())); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr const_iterator begin() const noexcept {
+ return const_iterator(std::addressof(this->__get()));
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr iterator end() noexcept { return begin() + this->has_value(); }
+ _LIBCPP_HIDE_FROM_ABI constexpr const_iterator end() const noexcept { return begin() + this->has_value(); }
+# 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());
diff --git a/libcxx/include/version b/libcxx/include/version
index d98049bd57046..88bceb9f5dc01 100644
--- a/libcxx/include/version
+++ b/libcxx/include/version
@@ -585,7 +585,7 @@ __cpp_lib_void_t 201411L <type_traits>
# define __cpp_lib_mdspan 202406L
# undef __cpp_lib_not_fn
# define __cpp_lib_not_fn 202306L
-// # define __cpp_lib_optional_range_support 202406L
+# define __cpp_lib_optional_range_support 202406L
# undef __cpp_lib_out_ptr
# define __cpp_lib_out_ptr 202311L
// # define __cpp_lib_philox_engine 202406L
diff --git a/libcxx/modules/std/optional.inc b/libcxx/modules/std/optional.inc
index 0f812bc0e24a4..9ee51117277ce 100644
--- a/libcxx/modules/std/optional.inc
+++ b/libcxx/modules/std/optional.inc
@@ -10,7 +10,12 @@
export namespace std {
// [optional.optional], class template optional
using std::optional;
-
+#if _LIBCPP_STD_VER >= 26
+ // [optional.iterators], iterator support
+ namespace ranges {
+ using std::ranges::enable_view;
+ }
+#endif
// [optional.nullopt], no-value state indicator
using std::nullopt;
using std::nullopt_t;
@@ -18,6 +23,10 @@ export namespace std {
// [optional.bad.access], class bad_optional_access
using std::bad_optional_access;
+#if _LIBCPP_STD_VER >= 26
+ using std::format_kind;
+#endif
+
// [optional.relops], relational operators
using std::operator==;
using std::operator!=;
diff --git a/libcxx/test/libcxx/transitive_includes/cxx26.csv b/libcxx/test/libcxx/transitive_includes/cxx26.csv
index 5f906338f4b7c..7d383d5c8e819 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx26.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx26.csv
@@ -2,6 +2,7 @@ algorithm cctype
algorithm climits
algorithm compare
algorithm cstdint
+algorithm cstdio
algorithm cstring
algorithm ctime
algorithm cwchar
@@ -11,6 +12,8 @@ algorithm iosfwd
algorithm limits
algorithm optional
algorithm ratio
+algorithm stdexcept
+algorithm string_view
algorithm tuple
algorithm version
any cstdint
@@ -330,26 +333,32 @@ flat_map cctype
flat_map climits
flat_map compare
flat_map cstdint
+flat_map cstdio
flat_map cstring
flat_map cwchar
flat_map cwctype
flat_map initializer_list
+flat_map iosfwd
flat_map limits
flat_map optional
flat_map stdexcept
+flat_map string_view
flat_map tuple
flat_map version
flat_set cctype
flat_set climits
flat_set compare
flat_set cstdint
+flat_set cstdio
flat_set cstring
flat_set cwchar
flat_set cwctype
flat_set initializer_list
+flat_set iosfwd
flat_set limits
flat_set optional
flat_set stdexcept
+flat_set string_view
flat_set tuple
flat_set version
format array
@@ -417,13 +426,16 @@ functional array
functional cctype
functional compare
functional cstdint
+functional cstdio
functional cstring
functional cwchar
functional cwctype
functional initializer_list
+functional iosfwd
functional limits
functional optional
functional stdexcept
+functional string_view
functional tuple
functional typeinfo
functional unordered_map
@@ -623,13 +635,16 @@ locale version
map cctype
map compare
map cstdint
+map cstdio
map cstring
map cwchar
map cwctype
map initializer_list
+map iosfwd
map limits
map optional
map stdexcept
+map string_view
map tuple
map version
mdspan array
@@ -673,22 +688,36 @@ mutex typeinfo
mutex version
new version
numbers version
+numeric cctype
numeric climits
numeric compare
numeric cstdint
+numeric cstdio
numeric cstring
numeric ctime
+numeric cwchar
+numeric cwctype
numeric initializer_list
+numeric iosfwd
numeric limits
numeric optional
numeric ratio
+numeric stdexcept
+numeric string_view
numeric tuple
numeric version
+optional cctype
optional compare
optional cstdint
+optional cstdio
optional cstring
+optional cwchar
+optional cwctype
optional initializer_list
+optional iosfwd
optional limits
+optional stdexcept
+optional string_view
optional version
ostream array
ostream bitset
@@ -806,6 +835,7 @@ ranges limits
ranges optional
ranges span
ranges stdexcept
+ranges string_view
ranges tuple
ranges variant
ranges version
@@ -851,12 +881,16 @@ semaphore version
set cctype
set compare
set cstdint
+set cstdio
set cstring
set cwchar
set cwctype
set initializer_list
+set iosfwd
set limits
set optional
+set stdexcept
+set string_view
set tuple
set version
shared_mutex cerrno
@@ -1091,21 +1125,34 @@ typeindex typeinfo
typeindex version
typeinfo cstdint
typeinfo version
+unordered_map cctype
unordered_map compare
unordered_map cstdint
+unordered_map cstdio
unordered_map cstring
+unordered_map cwchar
+unordered_map cwctype
unordered_map initializer_list
+unordered_map iosfwd
unordered_map limits
unordered_map optional
unordered_map stdexcept
+unordered_map string_view
unordered_map tuple
unordered_map version
+unordered_set cctype
unordered_set compare
unordered_set cstdint
+unordered_set cstdio
unordered_set cstring
+unordered_set cwchar
+unordered_set cwctype
unordered_set initializer_list
+unordered_set iosfwd
unordered_set limits
unordered_set optional
+unordered_set stdexcept
+unordered_set string_view
unordered_set tuple
unordered_set version
utility compare
diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/optional.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/optional.version.compile.pass.cpp
index 148a6dbc0d3e4..f5a05ee300dd7 100644
--- a/libcxx/test/std/language.support/support.limits/support.limits.general/optional.version.compile.pass.cpp
+++ b/libcxx/test/std/language.support/support.limits/support.limits.general/optional.version.compile.pass.cpp
@@ -152,17 +152,11 @@
# error "__cpp_lib_optional should have the value 202110L in c++26"
# endif
-# if !defined(_LIBCPP_VERSION)
-# ifndef __cpp_lib_optional_range_support
-# error "__cpp_lib_optional_range_support should be defined in c++26"
-# endif
-# if __cpp_lib_optional_range_support != 202406L
-# error "__cpp_lib_optional_range_support should have the value 202406L in c++26"
-# endif
-# else
-# ifdef __cpp_lib_optional_range_support
-# error "__cpp_lib_optional_range_support should not be defined because it is unimplemented in libc++!"
-# endif
+# ifndef __cpp_lib_optional_range_support
+# error "__cpp_lib_optional_range_support should be defined in c++26"
+# endif
+# if __cpp_lib_optional_range_support != 202406L
+# error "__cpp_lib_optional_range_support should have the value 202406L in c++26"
# endif
#endif // TEST_STD_VER > 23
diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
index 962688e06188a..ba445248a711f 100644
--- a/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
+++ b/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
@@ -7443,17 +7443,11 @@
# error "__cpp_lib_optional should have the value 202110L in c++26"
# endif
-# if !defined(_LIBCPP_VERSION)
-# ifndef __cpp_lib_optional_range_support
-# error "__cpp_lib_optional_range_support should be defined in c++26"
-# endif
-# if __cpp_lib_optional_range_support != 202406L
-# error "__cpp_lib_optional_range_support should have the value 202406L in c++26"
-# endif
-# else
-# ifdef __cpp_lib_optional_range_support
-# error "__cpp_lib_optional_range_support should not be defined because it is unimplemented in libc++!"
-# endif
+# ifndef __cpp_lib_optional_range_support
+# error "__cpp_lib_optional_range_support should be defined in c++26"
+# endif
+# if __cpp_lib_optional_range_support != 202406L
+# error "__cpp_lib_optional_range_support should have the value 202406L in c++26"
# endif
# ifndef __cpp_lib_out_ptr
diff --git a/libcxx/test/std/utilities/optional/iterator.pass.cpp b/libcxx/test/std/utilities/optional/iterator.pass.cpp
new file mode 100644
index 0000000000000..d9add15128ece
--- /dev/null
+++ b/libcxx/test/std/utilities/optional/iterator.pass.cpp
@@ -0,0 +1,124 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 T> class optional::iterator;
+
+// 1: optional::iterator and optional const_iterator satisfy contiguous_iterator and random_access_iterator
+// 2. The value types and reference types for optional::iterator and optional::const_iterator are {_Tp, _Tp&} and {_Tp, const _Tp&} respectively
+// 3: The optional::begin() and optional::end() are marked noexcept.
+// 4: optionals that have a value have begin() != end(), whereas one that doesn't has begin() == end();
+// 5: The corresponding size for the following optionals is respected: has_value() == 1, !has_value() == 0
+// 6: Dereferencing an engaged optional's iterator returns the correct value.
+// 7: std::ranges::enable_view<optional<T>> == true, and std::format_kind<optional<T>> == true
+// 8: Verify that an iterator for loop counts only 1 item for an engaged optional, and 0 for an unegaged one.
+// 9: 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.
+
+#include <cassert>
+#include <optional>
+
+#include "test_macros.h"
+#include "check_assertion.h"
+
+constexpr int test_loop(const std::optional<char> val) {
+ int size = 0;
+ for (auto&& v : val) {
+ std::ignore = v;
+ size++;
+ }
+ return size;
+}
+
+constexpr bool test_reset() {
+ std::optional<int> val{1};
+ assert(val.begin() != val.end());
+ val.reset();
+ assert(val.begin() == val.end());
+ val = 1;
+ assert(val.begin() != val.end());
+ assert(*(val.begin()) == 1);
+ return true;
+}
+
+int main(int, char**) {
+ constexpr const std::optional<char> opt{'a'};
+ constexpr std::optional<char> unengaged_opt;
+ std::optional<char> nonconst_opt{'n'};
+
+ // 1
+ {
+ static_assert(std::contiguous_iterator<decltype(nonconst_opt.begin())>);
+ static_assert(std::contiguous_iterator<decltype(nonconst_opt.end())>);
+ static_assert(std::random_access_iterator<decltype(nonconst_opt.begin())>);
+ static_assert(std::random_access_iterator<decltype(nonconst_opt.end())>);
+
+ static_assert(std::contiguous_iterator<decltype(opt.begin())>);
+ static_assert(std::contiguous_iterator<decltype(opt.end())>);
+ static_assert(std::random_access_iterator<decltype(opt.begin())>);
+ static_assert(std::random_access_iterator<decltype(opt.end())>);
+ }
+
+ { // 2
+ static_assert(std::same_as<typename decltype(opt.begin())::value_type, char>);
+ static_assert(std::same_as<typename decltype(opt.begin())::reference, const char&>);
+ static_assert(std::same_as<typename decltype(nonconst_opt.begin())::value_type, char>);
+ static_assert(std::same_as<typename decltype(nonconst_opt.begin())::reference, char&>);
+ }
+
+ // 3
+ {
+ ASSERT_NOEXCEPT(opt.begin());
+ ASSERT_NOEXCEPT(opt.end());
+ ASSERT_NOEXCEPT(nonconst_opt.begin());
+ ASSERT_NOEXCEPT(nonconst_opt.end());
+ }
+
+ { // 4
+ static_assert(opt.begin() != opt.end());
+ static_assert(unengaged_opt.begin() == unengaged_opt.end());
+ assert(unengaged_opt.begin() == unengaged_opt.end());
+ assert(opt.begin() != opt.end());
+ assert(nonconst_opt.begin() != opt.end());
+ }
+
+ // 5
+ {
+ static_assert(std::ranges::size(opt) == 1);
+ static_assert(std::ranges::size(unengaged_opt) == 0);
+ assert(std::ranges::size(opt) == 1);
+ assert(std::ranges::size(unengaged_opt) == 0);
+ }
+
+ { // 6
+ static_assert(*opt.begin() == 'a');
+ assert(*(opt.begin()) == 'a');
+ assert(*(nonconst_opt.begin()) == 'n');
+ }
+
+ { // 7
+ static_assert(std::ranges::enable_view<std::optional<char>> == true);
+ assert(std::format_kind<std::optional<char>> == std::range_format::disabled);
+ }
+
+ { // 8
+ static_assert(test_loop(opt) == 1);
+ static_assert(test_loop(unengaged_opt) == 0);
+ assert(test_loop(opt) == 1);
+ assert(test_loop(unengaged_opt) == 0);
+ }
+
+ { // 9
+ static_assert(test_reset());
+ assert(test_reset());
+ }
+
+ return 0;
+}
diff --git a/libcxx/utils/generate_feature_test_macro_components.py b/libcxx/utils/generate_feature_test_macro_components.py
index fe175fd758726..152adfbbc52da 100644
--- a/libcxx/utils/generate_feature_test_macro_components.py
+++ b/libcxx/utils/generate_feature_test_macro_components.py
@@ -1014,7 +1014,6 @@ def add_version_header(tc):
"name": "__cpp_lib_optional_range_support",
"values": {"c++26": 202406}, # P3168R2 Give std::optional ...
[truncated]
|
|
Thanks for working on this. A nice pre-requisit to |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
smallp-o-p
force-pushed
the
implement-P3168R2
branch
from
2281d68 to
843f616
Compare
| public: | ||
| using value_type = _Tp; | ||
| # if _LIBCPP_STD_VER >= 26 | ||
| using iterator = __wrap_iter<pointer>; |
There was a problem hiding this comment.
I wonder if __wrap_iter is the right tool for the job here. __wrap_iter is basically __random_access_iterator except it has a weird name due to historical reasons. Recently, we've been trying to maintain relevant static information in the type of the iterators, see for example this discussion or this one yielding this issue.
As you have it right now, std::optional iterators would be interchangeable with std::vector iterators. I'm not certain that's what we want since that could lead to unintended misuse from users, and we're also dropping a lot of information on the floor (namely the fact that the range's size is at most 1).
My current thinking is that we should either:
- Define an iterator that is specific to
std::optional, or - Define an iterator type that encodes the static information about the maximum size of the range being
N. Something like__statically_bounded_iterator<Underlying, N>, although that name clashes withlibcxx/include/__iterator/static_bounded_iter.hwhich is used for hardening. The idea is similar, except the iterator used byoptionaldoesn't need to physically maintain its bounds, it just wants to encode that static knowledge somewhere.
This actually makes me realize that there's something else we should probably do as well: we should probably provide an option to use hardened __static_bounded_iters in optional (behind an ABI macro).
There was a problem hiding this comment.
I personally think the second option sounds good, but should go in a different PR of course. It might also be useful for other containers like inplace_vector.
As for the __static_bounded_iter suggestion, I originally implemented hardening with __bounded_iter (but kept it out of the PR to get some input first), which felt like the better choice since we can't know if the optional is going to have a range of 0 or 1 at compile time(?)
There was a problem hiding this comment.
@ldionne What improvement do you see by providing an optional-specific iterator? I don't really see how such an iterator would buy us anything. It would be an improvement for bounds-safety, but that's not provided here anyways.
There was a problem hiding this comment.
Considering optional<T(&)[]> (note the missing bound) and optional<T(&)(Args)>, I wonder whether they should have iterator. Certainly, such an iterator can't be a contiguous iterator (and not even a real iterator in the case of optional<T(&)(Args)>), and our wrappers don't seem to work with them. If we decide they should have iterator, we probably need to introduce optional-specific iterators. @philnik777 @ldionne
There was a problem hiding this comment.
@philnik777 The improvement I see by having an optional-specific iterator is that we can't mistake it for a std::vector::iterator, for example. There might also be optimization opportunities by knowing that an iterator points to at most one element (basically that encodes the knowledge of a range having at most N elements where N is a compile-time constant). But I'm primarily interested in making the following fail:
std::vector<int> v = {...};
std::optional<int> opt = ...;
std::sort(v.begin(), opt.end()); // wtf?
If we use __wrap_iter, that code would compile despite being obviously wrong. A bunch of other stuff would also work, such as v.begin() != opt.end(), and none of it makes sense.
There was a problem hiding this comment.
@frederick-vs-ja I fail to see what's wrong with the optional types you mentioned. Is it because you can't have a reference to a function as an iterator's value_type?
There was a problem hiding this comment.
which felt like the better choice since we can't know if the optional is going to have a range of 0 or 1 at compile time(?)
Yes, you're right, __static_bounded_iter doesn't work for that reason. I think your current usage of __bounded_iter is the best we can do for now.
| assert(test<int>()); | ||
| assert(test<char>()); | ||
| assert(test<const int>()); | ||
| assert(test<const char>()); |
There was a problem hiding this comment.
I think I like how you organized the tests now. FYI only - I am not asking for any changes: there is an alternative to iterate over multiple types a helper types::for_each(types::integer_types(), IntegerTypesTest{}); an example usage is here: https://github.com/llvm/llvm-project/blob/9ae7490c6251ec60ab978bb818e89ca586e0c9c9/libcxx/test/std/ranges/range.factories/range.iota.view/indices.pass.cpp
…r inclusion in __pstl/default.h in C++17 and C++20 The circular include looks like the following: optional -> __format/range_format.h -> __format/concepts.h -> __format/format_parse_context.h -> string_view -> algorithm -> __algorithm/pstl.h -> __pstl/backend.h -> __pstl/backends/default.h -> optional
…2, also remove check_assertion.h header in test due to it breaking
…nce they weren't necessary and were messing with _Tp of const types
smallp-o-p
force-pushed
the
implement-P3168R2
branch
from
bdcc6ef to
e9ec733
Compare
There was a problem hiding this comment.
This is looking very good!
I would be okay with moving forward with this patch after my small comments have been addressed, but I would like a commitment to introduce an iterator that is specific to optional in the near future (in particular before the LLVM 22 release). If we don't have an iterator that is specific to optional, we can't ship this in a release because this is going to be ABI once we ship it.
| public: | ||
| using value_type = _Tp; | ||
| # if _LIBCPP_STD_VER >= 26 | ||
| using iterator = __wrap_iter<pointer>; |
There was a problem hiding this comment.
which felt like the better choice since we can't know if the optional is going to have a range of 0 or 1 at compile time(?)
Yes, you're right, __static_bounded_iter doesn't work for that reason. I think your current usage of __bounded_iter is the best we can do for now.
|
|
||
| template <typename T> | ||
| constexpr bool test() { | ||
| constexpr std::optional<T> opt{T{}}; |
There was a problem hiding this comment.
Can you also add a test for calling begin() on an empty optional?
Also, this object should be const, not constexpr. We definitely want to be calling .begin() on a runtime object, we just want it to have a const qualifier.
|
|
||
| template <typename T> | ||
| constexpr bool test() { | ||
| std::optional<T> unengaged{std::nullopt}; |
There was a problem hiding this comment.
Not a native english speaker, but I think we generally use the term "disengaged". Nitpick.
| assert(noexcept(opt.begin())); | ||
| assert(noexcept(nonconst_opt.begin())); |
There was a problem hiding this comment.
One way that we sometimes avoid having to create two variables (one const and one mutable) is this:
std::optional<T> opt{T{}};
assert(noexcept(opt.begin()));
assert(noexcept(std::as_const(opt).begin()));
This is a pattern we have in a bunch of tests, IMO it makes things a bit clearer because it encodes the particularity of what we're testing on the same line as the assertion, but I'll leave it to your preference.
| return true; | ||
| } | ||
|
|
||
| int main() { |
There was a problem hiding this comment.
Nitpick: int main(int, char**).
The reason is subtle and not caught by our CI: in freestanding mode, the main function is not special, so it doesn't get extern "C" mangling automatically. That means that we need to normalize on a single overload of main to use throughout all of our tests if we want to be able to hook onto the main function of our tests in freestanding mode. Otherwise you'd get a linker error from whatever tries to call main in that setup (which would probably be something like a custom crt0.a thingy).
Like I said, it's a nitpick and it's very subtle, but it's good to be aware of it. It's also not the end of the world if we commit incorrect main functions, I catch them from time to time and just fix them in bulk.
For the same reason, main needs to return 0; explicitly (not done in your end.pass.cpp test) since there is no implicit return 0 from the main function in freestanding mode.
| constexpr std::optional<T> unengaged2{std::nullopt}; | ||
|
|
||
| { // end() is marked noexcept | ||
| assert(noexcept(unengaged.end())); |
There was a problem hiding this comment.
Since this is a pure compile-time property, we'd generally use static_assert(noexcept(unengaged.end())); here instead. Both are just as valid but that would be more consistent with what we do everywhere.
| assert((std::is_same_v<typename decltype(it2)::reference, T&>)); | ||
| } | ||
|
|
||
| { // std::ranges for an engaged optional<T> == 1, unengaged optional<T> == 0 |
There was a problem hiding this comment.
| { // std::ranges for an engaged optional<T> == 1, unengaged optional<T> == 0 | |
| { // std::ranges::size for an engaged optional<T> == 1, unengaged optional<T> == 0 |
| } | ||
|
|
||
| { // std::ranges for an engaged optional<T> == 1, unengaged optional<T> == 0 | ||
| constexpr std::optional<T> unengaged{std::nullopt}; |
There was a problem hiding this comment.
| constexpr std::optional<T> unengaged{std::nullopt}; | |
| const std::optional<T> unengaged{std::nullopt}; |
| assert(std::ranges::enable_view<std::optional<T>> == true); | ||
| assert(std::format_kind<std::optional<T>> == std::range_format::disabled); |
There was a problem hiding this comment.
| assert(std::ranges::enable_view<std::optional<T>> == true); | |
| assert(std::format_kind<std::optional<T>> == std::range_format::disabled); | |
| static_assert(std::ranges::enable_view<std::optional<T>> == true); | |
| static_assert(std::format_kind<std::optional<T>> == std::range_format::disabled); |
| assert(std::format_kind<std::optional<T>> == std::range_format::disabled); | ||
| } | ||
|
|
||
| // 8: 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. |
There was a problem hiding this comment.
| // 8: 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. | |
| // 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. |
Minor formatting: removing seemingly stray 8: and breaking the line.
I can get started with something like an |
Resolves #105430
wrap_iterclass to implement theoptionaliterator type. Hardening has not been added, do we want that foroptional?