[libcxx-commits] [libcxx] [libc++] Implement P3168R2: Give optional range support (PR #149441)

[Louis Dionne via libcxx-commits]

================
@@ -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>;
----------------
ldionne wrote:

Ah, that's a good point. That's LWG issue territory then, isn't it? What would you think of *not* providing the `iterator` typedef in the cases that we know the resulting typedef is not going to be a proper iterator? That would close this door for the time being, until WG21 decides what to do with it design-wise.

If you agree @frederick-vs-ja, then @smallp-o-p this patch should basically change to something like this:

```c++
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:

```c++
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 `libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp`. Notice how this is under `libcxx/test/libcxx` instead of `libcxx/test/std`: that is because this test would be specific to libc++ (until WG21 decides what's the Standard behavior). We put tests that check libc++ specific implementation details under a separate directory so that other standard libraries can reuse the `libcxx/test/std` directory to check their own conformance status.

Finally, we'd also need to make sure that we have *positive* testing coverage for `std::optional<int (&)[]>::pointer` and `std::optional<void (&)(int, char)>::pointer` under `libcxx/test/std`. That actually makes me realize that this patch is missing such coverage even for `std::optional<int>` (unless I missed it) -- so it should be added. We generally test such things in a test named `some/path/types.pass.cpp`, and it looks like we already have one for `std::optional` here: `libcxx/test/std/utilities/optional/optional.object/types.pass.cpp`. You will want to augment it with additional tests guarded under `TEST_STD_VER >= 26`, a macro that can be found in `test_macros.h`.

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.


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