[libc++] Implement a type-safe iterator for optional #154239
Open
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++
|
@llvm/pr-subscribers-libcxx Author: William Tran-Viet (smallp-o-p) ChangesAs promised to @ldionne when implementing P3168R2, a new, type-safe iterator for
The only thing I'm iffy about is the somewhat silly amounts of iterator wrapping when it comes to Full diff: https://github.com/llvm/llvm-project/pull/154239.diff 5 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index c6b87a34a43e9..6fedc58be1705 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -509,6 +509,7 @@ set(files
__iterator/sortable.h
__iterator/static_bounded_iter.h
__iterator/unreachable_sentinel.h
+ __iterator/upper_bounded_iterator.h
__iterator/wrap_iter.h
__locale
__locale_dir/check_grouping.h
diff --git a/libcxx/include/__iterator/upper_bounded_iterator.h b/libcxx/include/__iterator/upper_bounded_iterator.h
new file mode 100644
index 0000000000000..4a53f54c97530
--- /dev/null
+++ b/libcxx/include/__iterator/upper_bounded_iterator.h
@@ -0,0 +1,164 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+/*
+ * __upper_bounded_iterator is an iterator that wraps an underlying iterator.
+ * It stores the underlying container type to prevent mixing iterators, and allow algorithms
+ * to optimize based on the underlying container type.
+ * It also stores the absolute maximum amount of elements the container can have, known at compile-time.
+ * As of writing, the only standard library containers which have this property are inplace_vector and optional.
+ */
+
+#ifndef _LIBCPP___ITERATOR_UPPER_BOUNDED_ITERATOR_H
+#define _LIBCPP___ITERATOR_UPPER_BOUNDED_ITERATOR_H
+
+#include <__compare/three_way_comparable.h>
+#include <__config>
+#include <__cstddef/size_t.h>
+#include <__iterator/incrementable_traits.h>
+#include <__iterator/iterator_traits.h>
+#include <__memory/pointer_traits.h>
+#include <__type_traits/is_constructible.h>
+#include <__utility/move.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+#if _LIBCPP_STD_VER >= 26
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+template <class _Iter, class _Container, size_t _Max_Elements>
+class __upper_bounded_iterator {
+private:
+ _Iter __iter_;
+
+ friend _Container;
+
+public:
+ using iterator_category = iterator_traits<_Iter>::iterator_category;
+ using iterator_concept = _Iter::iterator_concept;
+ using value_type = iter_value_t<_Iter>;
+ using difference_type = iter_difference_t<_Iter>;
+ using reference = iter_reference_t<_Iter>;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator()
+ requires is_default_constructible_v<_Iter>
+ = default;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator(_Iter __iter) : __iter_(std::move(__iter)) {}
+
+ _LIBCPP_HIDE_FROM_ABI _Iter __base() const { return __iter_; }
+ _LIBCPP_HIDE_FROM_ABI constexpr size_t __max_elements() const { return _Max_Elements; }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr decltype(auto) operator*() const { return *__iter_; }
+ _LIBCPP_HIDE_FROM_ABI constexpr decltype(auto) operator->() const { return __iter_.operator->(); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator& operator++() {
+ ++__iter_;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator operator++(int) {
+ __upper_bounded_iterator __tmp(*this);
+ ++*this;
+ return __tmp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator& operator--() {
+ --__iter_;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator operator--(int) {
+ __upper_bounded_iterator __tmp(*this);
+ --*this;
+ return __tmp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator& operator+=(difference_type __x) {
+ __iter_ += __x;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __upper_bounded_iterator& operator-=(difference_type __x) {
+ __iter_ -= __x;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr decltype(auto) operator[](difference_type __n) const { return *(*this + __n); }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool
+ operator==(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y) {
+ return __x.__iter_ == __y.__iter_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool
+ operator<(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y) {
+ return __x.__iter_ < __y.__iter_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool
+ operator>(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y) {
+ return __y < __x;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool
+ operator<=(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y) {
+ return !(__y < __x);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool
+ operator>=(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y) {
+ return !(__x < __y);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr auto
+ operator<=>(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y)
+ requires three_way_comparable<_Iter>
+ {
+ return __x.__iter_ <=> __y.__iter_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr __upper_bounded_iterator
+ operator+(const __upper_bounded_iterator& __i, difference_type __n) {
+ auto __tmp = __i;
+ __tmp += __n;
+ return __tmp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr __upper_bounded_iterator
+ operator+(difference_type __n, const __upper_bounded_iterator& __i) {
+ return __i + __n;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr __upper_bounded_iterator
+ operator-(const __upper_bounded_iterator& __i, difference_type __n) {
+ auto __tmp = __i;
+ __tmp -= __n;
+ return __tmp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr difference_type
+ operator-(const __upper_bounded_iterator& __x, const __upper_bounded_iterator& __y) {
+ return __x.__iter_ - __y.__iter_;
+ }
+};
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP_STD_VER >= 26
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___ITERATOR_UPPER_BOUNDED_ITERATOR_H
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index c431c0cb407f3..78586e1154965 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -1545,6 +1545,7 @@ module std [system] {
module sortable { header "__iterator/sortable.h" }
module static_bounded_iter { header "__iterator/static_bounded_iter.h" }
module unreachable_sentinel { header "__iterator/unreachable_sentinel.h" }
+ module upper_bounded_iterator { header "__iterator/upper_bounded_iterator.h" }
module wrap_iter { header "__iterator/wrap_iter.h" }
header "iterator"
diff --git a/libcxx/include/optional b/libcxx/include/optional
index 39fcaa2c2ec18..193f85b855af4 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -199,7 +199,6 @@ 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>
@@ -207,6 +206,7 @@ namespace std {
# include <__functional/unary_function.h>
# include <__fwd/functional.h>
# include <__iterator/bounded_iter.h>
+# include <__iterator/upper_bounded_iterator.h>
# include <__iterator/wrap_iter.h>
# include <__memory/addressof.h>
# include <__memory/construct_at.h>
@@ -225,7 +225,6 @@ namespace std {
# include <__type_traits/is_convertible.h>
# include <__type_traits/is_core_convertible.h>
# include <__type_traits/is_destructible.h>
-# include <__type_traits/is_function.h>
# include <__type_traits/is_nothrow_assignable.h>
# include <__type_traits/is_nothrow_constructible.h>
# include <__type_traits/is_object.h>
@@ -238,7 +237,6 @@ namespace std {
# include <__type_traits/is_trivially_constructible.h>
# include <__type_traits/is_trivially_destructible.h>
# include <__type_traits/is_trivially_relocatable.h>
-# include <__type_traits/is_unbounded_array.h>
# include <__type_traits/negation.h>
# include <__type_traits/remove_const.h>
# include <__type_traits/remove_cv.h>
@@ -622,11 +620,11 @@ 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>>;
+ using iterator = __upper_bounded_iterator<__bounded_iter<__wrap_iter<__pointer>>, optional, 1>;
+ using const_iterator = __upper_bounded_iterator<__bounded_iter<__wrap_iter<__const_pointer>>, optional, 1>;
# else
- using iterator = __wrap_iter<__pointer>;
- using const_iterator = __wrap_iter<__const_pointer>;
+ using iterator = __upper_bounded_iterator<__wrap_iter<__pointer>, optional, 1>;
+ using const_iterator = __upper_bounded_iterator<__wrap_iter<__const_pointer>, optional, 1>;
# endif
# endif
using __trivially_relocatable _LIBCPP_NODEBUG =
@@ -841,7 +839,7 @@ public:
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()));
+ return iterator(__wrap_iter<__pointer>(std::addressof(this->__get())));
# endif
}
@@ -852,7 +850,7 @@ public:
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()));
+ return const_iterator(__wrap_iter<__const_pointer>(std::addressof(this->__get())));
# endif
}
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..b8c8dc37d0e41 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
@@ -7,13 +7,13 @@
//===----------------------------------------------------------------------===//
// REQUIRES: std-at-least-c++26
-
// <optional>
// template <class T> class optional::iterator;
// template <class T> class optional::const_iterator;
#include <optional>
+#include <type_traits>
template <typename T>
concept has_iterator_aliases = requires {
@@ -24,6 +24,31 @@ concept has_iterator_aliases = requires {
static_assert(has_iterator_aliases<std::optional<int>>);
static_assert(has_iterator_aliases<std::optional<const int>>);
+using Iter1 = std::optional<int>::iterator;
+using Iter2 = std::optional<double>::iterator;
+using Iter3 = std::optional<int>::const_iterator;
+using Iter4 = std::optional<double>::const_iterator;
+
+static_assert(std::is_convertible_v<Iter1, Iter1>);
+static_assert(!std::is_convertible_v<Iter1, Iter2>);
+static_assert(!std::is_convertible_v<Iter1, Iter3>);
+static_assert(!std::is_convertible_v<Iter1, Iter4>);
+
+static_assert(std::is_convertible_v<Iter2, Iter2>);
+static_assert(!std::is_convertible_v<Iter2, Iter1>);
+static_assert(!std::is_convertible_v<Iter2, Iter3>);
+static_assert(!std::is_convertible_v<Iter2, Iter4>);
+
+static_assert(std::is_convertible_v<Iter3, Iter3>);
+static_assert(!std::is_convertible_v<Iter3, Iter1>);
+static_assert(!std::is_convertible_v<Iter3, Iter2>);
+static_assert(!std::is_convertible_v<Iter3, Iter4>);
+
+static_assert(std::is_convertible_v<Iter4, Iter4>);
+static_assert(!std::is_convertible_v<Iter4, Iter1>);
+static_assert(!std::is_convertible_v<Iter4, Iter2>);
+static_assert(!std::is_convertible_v<Iter4, Iter3>);
+
// TODO: Uncomment these once P2988R12 is implemented, as they would be testing optional<T&>
// static_assert(!has_iterator_aliases<std::optional<int (&)[]>>);
|
Zingam
reviewed
Aug 19, 2025
There was a problem hiding this comment.
Thanks for working on this.
IMO You should keep the commit message (the top comment) terser, on point and less opionated, as you have been asked in the previous PR. You can give more context, etc. in subsequent comments.
I am not sure but do we need libc++ specific tests for this iterator? I think they need to be placed under test/libcxx instead of test/std.
libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp
Outdated
Show resolved
Hide resolved
Fair enough.
I'm not quite sure that I understand? Do you mean they need to be placed in |
On a second look, please, ignore my comment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As promised to @ldionne when implementing P3168R2: A type-safe iterator for
optional.__upper_bounded_iteratoriterator type which wraps an existing iterator, takes its container as a template parameter, and encodes the maximum amount of elements the container can hold. The main objective is to prevent iterator mixups between different containers (e.g.vector).optionalthat were mistakenly left in [libc++] Implement P3168R2: Give optional range support #149441.libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cppto verify type safety.