[libcxx-commits] [libcxx] [libc++] Remove `optional::iterator` from experimental mode (PR #203782)

[William Tran-Viet via libcxx-commits]

================
@@ -681,50 +682,77 @@ class _LIBCPP_DECLSPEC_EMPTY_BASES optional
 
   using __base::reset;
 
-#  if _LIBCPP_STD_VER >= 26 && _LIBCPP_HAS_EXPERIMENTAL_OPTIONAL_ITERATOR
+#  if _LIBCPP_STD_VER >= 26
 
 private:
   using __pointer _LIBCPP_NODEBUG       = add_pointer_t<_Tp>;
   using __const_pointer _LIBCPP_NODEBUG = add_pointer_t<const _Tp>;
 
-public:
+  template <class _Up = _Tp, bool = std::__range_fits_in_alignment(alignof(_Up), 1)>
+  struct __iterator {
 #    ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
-  using iterator       = __bounded_iter<__pointer>;
-  using const_iterator = __bounded_iter<__const_pointer>;
+    using __type _LIBCPP_NODEBUG       = std::__bounded_iter<__pointer>;
+    using __const_type _LIBCPP_NODEBUG = std::__bounded_iter<__const_pointer>;
 #    else
-  using iterator       = __capacity_aware_iterator<__pointer, 1>;
-  using const_iterator = __capacity_aware_iterator<__const_pointer, 1>;
+    using __type _LIBCPP_NODEBUG       = std::__static_packed_bounded_iterator<__pointer, 1>;
+    using __const_type _LIBCPP_NODEBUG = std::__static_packed_bounded_iterator<__const_pointer, 1>;
 #    endif
 
-  // [optional.iterators], iterator support
-  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr iterator begin() noexcept {
-    auto* __ptr = std::addressof(this->__val_);
-
+    template <class _PtrType>
+    static auto __make(_PtrType __p, [[__maybe_unused__]] unsigned __end_offset) noexcept {
 #    ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
-    return std::__make_bounded_iter(__ptr, __ptr, __ptr + (this->has_value() ? 1 : 0));
+      return std::__make_bounded_iter<_PtrType>(__p, __p, __p + __end_offset);
 #    else
-    return std::__make_capacity_aware_iterator<__pointer, 1>(__ptr);
+      return std::__make_static_packed_bounded_iter<_PtrType, 1>(__p, 0);
 #    endif
-  }
+    }
+  };
 
-  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr const_iterator begin() const noexcept {
-    auto* __ptr = std::addressof(this->__val_);
+  template <class _Up>
+  struct __iterator<_Up, false> {
+#    ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_OPTIONAL
+    using __type _LIBCPP_NODEBUG       = std::__bounded_iter<__pointer>;
----------------
smallp-o-p wrote:

> Either way, I thought we could use `__static_bounded_iter` here, but we can in fact not if we want the same guarantees as `__bounded_iter` provides. However, we can have a `__packed_bounded_iter` where we can again store the bounds in the low bits, or in two integers following the pointer if the low bits aren't enough. For `optional::iterator` this would save us 1-2 pointers worth of size, potentially allowing them to be passed in registers instead of on the stack. (I believe the current `__bounded_iter` always passed on the stack currently?)

I think a lot of the weirdness comes from `optional`'s irregular usage of `__bounded_iter`. We pass in the *current* size of the `optional`, rather than its capacity (which is just 1), deviating from the intended use case of `__bounded_iter` where it was written for `vector` which would pass in its current capacity pointer.

This usage does allow a stronger guarantee: We can assert when a user tries to dereference an iterator taken on an empty `optional`, which in this could would lead to an uninitialized read. Other containers which use `__bounded_iter` do not check whether its going past the actual size of the container like we do here, just that it's within owned memory.

If we're willing to let go of that extra guarantee, and bring `optional`'s bounds checking to be more "inline" with other containers (i.e. not checking if a user is trying to read a disengaged `optional`), then we could just replace `__bounded_iter` with `__static_bounded_iter` and call it a day.

This conversation and the decision we make here would also apply to `inplace_vector`.

https://github.com/llvm/llvm-project/pull/203782