[libcxx-commits] [libcxx] [libc++] Resolve LWG4308: std::optional<T&>::iterator can't be a contiguous iterator for some T (PR #173948)

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: William Tran-Viet (smallp-o-p)

<details>
<summary>Changes</summary>

Resolves #<!-- -->171345

- Constrains iterator to only be available if T is not an lvalue reference, or if it is T&, that T is an object type and is not an unbounded array
- Correct a static assert message as a drive-by
- Move the libcxx specific iterator test into the standard test

---
Full diff: https://github.com/llvm/llvm-project/pull/173948.diff


3 Files Affected:

- (modified) libcxx/include/optional (+4-3) 
- (removed) libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp (-29) 
- (modified) libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp (+18-1) 


``````````diff
diff --git a/libcxx/include/optional b/libcxx/include/optional
index 440fdf73a4310..388a47487d512 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -499,7 +499,7 @@ struct __optional_storage_base<_Tp, true> {
   _LIBCPP_HIDE_FROM_ABI constexpr explicit __optional_storage_base(in_place_t, _UArg&& __uarg)
       : __value_(std::addressof(__uarg)) {
     static_assert(!__reference_constructs_from_temporary_v<_Tp, _UArg>,
-                  "Attempted to construct a reference element in tuple from a "
+                  "Attempted to construct a reference element in optional from a "
                   "possible temporary");
   }
 
@@ -681,11 +681,12 @@ struct __is_std_optional<optional<_Tp>> : true_type {};
 template <class _Tp, class = void>
 struct __optional_iterator {};
 
+// LWG4308 optional<T&>: Constraint: T is an object type other than an array of unknown bound.
 template <class _Tp>
 struct __optional_iterator<
     _Tp,
-    enable_if_t<!(is_lvalue_reference_v<_Tp> && is_function_v<__libcpp_remove_reference_t<_Tp>>) &&
-                !(is_lvalue_reference_v<_Tp> && is_array_v<__libcpp_remove_reference_t<_Tp>>)> > {
+    enable_if_t<!is_lvalue_reference_v<_Tp> || (is_object_v<__libcpp_remove_reference_t<_Tp>> &&
+                                                !__is_unbounded_array_v<__libcpp_remove_reference_t<_Tp>>)>> {
 private:
   using __pointer _LIBCPP_NODEBUG       = add_pointer_t<remove_reference_t<_Tp>>;
   using __const_pointer _LIBCPP_NODEBUG = add_pointer_t<const remove_reference_t<_Tp>>;
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
deleted file mode 100644
index b604579e43557..0000000000000
--- a/libcxx/test/libcxx/utilities/optional/optional.iterator/iterator.compile.pass.cpp
+++ /dev/null
@@ -1,29 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// 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;
-// template <class T> class optional::const_iterator;
-
-#include <optional>
-
-template <typename 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<const int>>);
-static_assert(has_iterator_aliases<std::optional<int&>>);
-static_assert(has_iterator_aliases<std::optional<const int&>>);
-static_assert(!has_iterator_aliases<std::optional<int (&)[1]>>);
-static_assert(!has_iterator_aliases<std::optional<int (&)()>>);
diff --git a/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp b/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp
index 671fac35e732a..909969144063d 100644
--- a/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.iterator/iterator.pass.cpp
@@ -19,6 +19,12 @@
 #include <type_traits>
 #include <utility>
 
+template <typename T>
+concept has_iterator = requires {
+  typename T::iterator;
+  typename T::const_iterator;
+};
+
 template <typename T>
 constexpr bool test_range_concept() {
   return std::ranges::range<std::optional<T>>;
@@ -90,6 +96,17 @@ constexpr bool test() {
 }
 
 constexpr bool tests() {
+  // Verify that iterator is present for object type T, but for T&, that iterator is available only if T is
+  // an object type and is not an unbounded array.
+
+  static_assert(has_iterator<std::optional<int>>);
+  static_assert(has_iterator<std::optional<const int>>);
+  static_assert(has_iterator<std::optional<int&>>);
+  static_assert(has_iterator<std::optional<const int&>>);
+  static_assert(has_iterator<std::optional<int (&)[1]>>);
+  static_assert(!has_iterator<std::optional<int (&)[]>>);
+  static_assert(!has_iterator<std::optional<int (&)()>>);
+
   assert((test<int, 1>()));
   assert((test<char, 'a'>()));
   assert((test<bool, true>()));
@@ -103,7 +120,7 @@ constexpr bool tests() {
 
   assert(!test_range_concept<int (&)()>());
   assert(!test_range_concept<int (&)[]>());
-  assert(!test_range_concept<int (&)[42]>());
+  assert(test_range_concept<int (&)[42]>());
 
   return true;
 }

``````````

</details>


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