Skip to content

Conversation

@ldionne
Copy link
Member

@ldionne ldionne commented Nov 7, 2024

I don't think there was any reason to use enable_if to enforce that bounded_iter and static_bounded_iter are instantiated with contiguous iterators. Using a static_assert removes that detail from the ABI and allows providing a slightly cleaner error message.

Fixes #115002

I don't think there was any reason to use enable_if to enforce that
bounded_iter and static_bounded_iter are instantiated with contiguous
iterators. Using a static_assert removes that detail from the ABI and
allows providing a slightly cleaner error message.

Fixes llvm#115002
@ldionne ldionne requested a review from philnik777 November 7, 2024 14:25
@ldionne ldionne requested a review from a team as a code owner November 7, 2024 14:25
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

I don't think there was any reason to use enable_if to enforce that bounded_iter and static_bounded_iter are instantiated with contiguous iterators. Using a static_assert removes that detail from the ABI and allows providing a slightly cleaner error message.

Fixes #115002


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

2 Files Affected:

  • (modified) libcxx/include/__iterator/bounded_iter.h (+5-2)
  • (modified) libcxx/include/__iterator/static_bounded_iter.h (+4-1)
diff --git a/libcxx/include/__iterator/bounded_iter.h b/libcxx/include/__iterator/bounded_iter.h
index 4811b0dde7f777..9b8532d55a331d 100644
--- a/libcxx/include/__iterator/bounded_iter.h
+++ b/libcxx/include/__iterator/bounded_iter.h
@@ -47,8 +47,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 //    pointer, it is undefined at the language level (see [expr.add]). If
 //    bounded iterators exhibited this undefined behavior, we risk compiler
 //    optimizations deleting non-redundant bounds checks.
-template <class _Iterator, class = __enable_if_t< __libcpp_is_contiguous_iterator<_Iterator>::value > >
+template <class _Iterator>
 struct __bounded_iter {
+  static_assert(__libcpp_is_contiguous_iterator<_Iterator>::value,
+                "__bounded_iter<It> requires It to be a contiguous iterator.");
+
   using value_type        = typename iterator_traits<_Iterator>::value_type;
   using difference_type   = typename iterator_traits<_Iterator>::difference_type;
   using pointer           = typename iterator_traits<_Iterator>::pointer;
@@ -67,7 +70,7 @@ struct __bounded_iter {
   _LIBCPP_HIDE_FROM_ABI __bounded_iter(__bounded_iter const&) = default;
   _LIBCPP_HIDE_FROM_ABI __bounded_iter(__bounded_iter&&)      = default;
 
-  template <class _OtherIterator, __enable_if_t< is_convertible<_OtherIterator, _Iterator>::value, int> = 0>
+  template <class _OtherIterator, __enable_if_t<is_convertible<_OtherIterator, _Iterator>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __bounded_iter(__bounded_iter<_OtherIterator> const& __other) _NOEXCEPT
       : __current_(__other.__current_),
         __begin_(__other.__begin_),
diff --git a/libcxx/include/__iterator/static_bounded_iter.h b/libcxx/include/__iterator/static_bounded_iter.h
index 2b80507cf56a01..b2e9e9853673fd 100644
--- a/libcxx/include/__iterator/static_bounded_iter.h
+++ b/libcxx/include/__iterator/static_bounded_iter.h
@@ -70,8 +70,11 @@ struct __static_bounded_iter_storage<_Iterator, 0> {
 // it can be computed from the start of the range.
 //
 // The operations on which this iterator wrapper traps are the same as `__bounded_iter`.
-template <class _Iterator, size_t _Size, class = __enable_if_t<__libcpp_is_contiguous_iterator<_Iterator>::value> >
+template <class _Iterator, size_t _Size>
 struct __static_bounded_iter {
+  static_assert(__libcpp_is_contiguous_iterator<_Iterator>::value,
+                "__static_bounded_iter<It> requires It to be a contiguous iterator.");
+
   using value_type        = typename iterator_traits<_Iterator>::value_type;
   using difference_type   = typename iterator_traits<_Iterator>::difference_type;
   using pointer           = typename iterator_traits<_Iterator>::pointer;

@ldionne
Copy link
Member Author

ldionne commented Nov 7, 2024

This is redundant, I had not seen #115304 yet.

@ldionne ldionne closed this Nov 7, 2024
@ldionne ldionne deleted the review/bounded_iter-static-assert-instead-of-enable_if branch November 7, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[libc++] __bounded_iter should use static_assert instead of enable_if for checking contiguous_iterator

2 participants