-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[libc++] Implement P3168R2: Give optional range support #149441
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
base: main
Are you sure you want to change the base?
Changes from all commits
ac4dcbc
3185aa3
60f67b2
a7740e8
02cdb07
913229a
559825b
3baa163
e3398ae
3b015e7
c8318f1
ebdc19d
60bf66b
3683469
e9ec733
97f3a7b
7cab021
c0ff08a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -186,13 +199,18 @@ namespace std { | |
# include <__compare/three_way_comparable.h> | ||
# include <__concepts/invocable.h> | ||
# include <__config> | ||
# include <__cstddef/ptrdiff_t.h> | ||
# include <__exception/exception.h> | ||
# include <__format/range_format.h> | ||
# include <__functional/hash.h> | ||
# include <__functional/invoke.h> | ||
# include <__functional/unary_function.h> | ||
# include <__fwd/functional.h> | ||
# include <__iterator/bounded_iter.h> | ||
# include <__iterator/wrap_iter.h> | ||
# include <__memory/addressof.h> | ||
# include <__memory/construct_at.h> | ||
# include <__ranges/enable_view.h> | ||
# include <__tuple/sfinae_helpers.h> | ||
# include <__type_traits/add_pointer.h> | ||
# include <__type_traits/conditional.h> | ||
|
@@ -567,6 +585,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 +612,22 @@ 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<_Tp>; | ||
using const_pointer = std::add_pointer_t<const _Tp>; | ||
# endif | ||
|
||
public: | ||
using value_type = _Tp; | ||
# if _LIBCPP_STD_VER >= 26 | ||
# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL | ||
using iterator = __bounded_iter<__wrap_iter<pointer>>; | ||
using const_iterator = __bounded_iter<__wrap_iter<const_pointer>>; | ||
# else | ||
using iterator = __wrap_iter<pointer>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if As you have it right now, My current thinking is that we should either:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 As for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ldionne What improvement do you see by providing an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @philnik777 The improvement I see by having an optional-specific iterator is that we can't mistake it for a
If we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. And for [optional.ref.iterators]/1 requires some weird I'm not sure whether we want these for (auto& fun : opt_func_ref) {
fun(args);
} But I'm not sure whether we want to support or disallow this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that's a good point. That's LWG issue territory then, isn't it? What would you think of not providing the If you agree @frederick-vs-ja, then @smallp-o-p this patch should basically change to something like this: template <class _Tp, class = void>
struct __optional_iterator_aliases { };
template <class _Tp>
struct __optional_iterator_aliases<_Tp, __enable_if_t<!is_function<__libcpp_remove_reference_t<_Tp> >::value &&
!is_unbounded_array<__libcpp_remove_reference_t<_Tp> >::value> > {
private:
// note: I am purposefully using private names here because I believe we want
// to provide ::pointer and ::const_pointer unconditionally.
using __pointer = std::add_pointer_t<_Tp>;
using __const_pointer = std::add_pointer_t<const _Tp>;
public:
# if _LIBCPP_STD_VER >= 26
# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
using iterator = __bounded_iter<__wrap_iter<__pointer>>;
using const_iterator = __bounded_iter<__wrap_iter<__const_pointer>>;
# else
using iterator = __wrap_iter<__pointer>;
using const_iterator = __wrap_iter<__const_pointer>;
# endif
# 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_aliases<_Tp> {
// ... We would also want to add a negative test like this one: template <class T>
concept has_iterator_aliases = requires {
typename T::iterator;
typename T::const_iterator;
};
static_assert(!has_iterator_aliases<std::optional<int (&)[]>>);
static_assert(!has_iterator_aliases<std::optional<void (&)(int, char)>>); That test would need to live under Finally, we'd also need to make sure that we have positive testing coverage for Sorry if you already know some of that, I'm trying to give you as much context as possible since I know this is amongst your first patches. |
||
using const_iterator = __wrap_iter<const_pointer>; | ||
# endif | ||
# endif | ||
|
||
using __trivially_relocatable _LIBCPP_NODEBUG = | ||
conditional_t<__libcpp_is_trivially_relocatable<_Tp>::value, optional, void>; | ||
|
@@ -792,6 +832,34 @@ 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()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// 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> | ||
|
||
// constexpr iterator optional::begin() noexcept; | ||
// constexpr const_iterator optional::begin() noexcept; | ||
|
||
#include <cassert> | ||
#include <iterator> | ||
#include <optional> | ||
#include <type_traits> | ||
#include <utility> | ||
|
||
template <typename T> | ||
constexpr bool test() { | ||
const std::optional<T> opt{T{}}; | ||
std::optional<T> nonconst_opt{T{}}; | ||
|
||
{ // begin() is marked noexcept | ||
static_assert(noexcept(opt.begin())); | ||
static_assert(noexcept(nonconst_opt.begin())); | ||
} | ||
|
||
{ // Dereferencing an iterator at the beginning == indexing the 0th element, and that calling begin() again return the same iterator. | ||
auto iter1 = opt.begin(); | ||
auto iter2 = nonconst_opt.begin(); | ||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both of these optionals are non-const. I believe that using |
||
assert(*iter1 == iter1[0]); | ||
assert(*iter2 == iter2[0]); | ||
assert(iter1 == opt.begin()); | ||
assert(iter2 == nonconst_opt.begin()); | ||
} | ||
|
||
{ // Calling begin() multiple times on a disengaged optional returns the same iterator. | ||
std::optional<T> disengaged{std::nullopt}; | ||
auto iter1 = disengaged.begin(); | ||
auto iter2 = std::as_const(disengaged).begin(); | ||
assert(iter1 == disengaged.begin()); | ||
assert(iter2 == std::as_const(disengaged).begin()); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
constexpr bool tests() { | ||
assert(test<int>()); | ||
assert(test<char>()); | ||
assert(test<const int>()); | ||
assert(test<const char>()); | ||
return true; | ||
} | ||
|
||
int main(int, char**) { | ||
assert(tests()); | ||
static_assert(tests()); | ||
|
||
return 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// 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> | ||
|
||
// constexpr iterator optional::end() noexcept; | ||
// constexpr const_iterator optional::end() noexcept; | ||
|
||
#include <cassert> | ||
#include <iterator> | ||
#include <ranges> | ||
#include <optional> | ||
|
||
template <typename T> | ||
constexpr bool test() { | ||
std::optional<T> disengaged{std::nullopt}; | ||
const std::optional<T> disengaged2{std::nullopt}; | ||
|
||
{ // end() is marked noexcept | ||
static_assert(noexcept(disengaged.end())); | ||
static_assert(noexcept(disengaged2.end())); | ||
} | ||
|
||
{ // end() == begin() and end() == end() if the optional is disengaged | ||
auto it = disengaged.end(); | ||
auto it2 = disengaged2.end(); | ||
|
||
assert(it == disengaged.begin()); | ||
assert(disengaged.begin() == it); | ||
assert(it == disengaged.end()); | ||
|
||
assert(it2 == disengaged2.begin()); | ||
assert(disengaged2.begin() == it2); | ||
assert(it2 == disengaged2.end()); | ||
} | ||
|
||
std::optional<T> engaged{T{}}; | ||
const std::optional<T> engaged2{T{}}; | ||
|
||
{ // end() != begin() if the optional is engaged | ||
auto it = engaged.end(); | ||
auto it2 = engaged2.end(); | ||
|
||
assert(it != engaged.begin()); | ||
assert(engaged.begin() != it); | ||
|
||
assert(it2 != engaged2.begin()); | ||
assert(engaged2.begin() != it2); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
constexpr bool tests() { | ||
assert(test<int>()); | ||
assert(test<char>()); | ||
assert(test<const int>()); | ||
assert(test<const char>()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
return true; | ||
} | ||
|
||
int main(int, char**) { | ||
assert(tests()); | ||
static_assert(tests()); | ||
|
||
return 0; | ||
} |
Uh oh!
There was an error while loading. Please reload this page.