[libc++] Disallow character types being index types of `extents` #105832

frederick-vs-ja · 2024-08-23T13:45:40Z

#78086 provided the IMO exact approach - __libcpp_integer - for this. Fixes #73715.

In some libcxx/containers/views/mdspan tests, improper uses of char are replaced with signed char.

llvmbot · 2024-08-23T13:46:12Z

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

#78086 provided the IMO exact approach - __libcpp_integer - for this. Fixes #73715.

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

2 Files Affected:

(modified) libcxx/include/__mdspan/extents.h (+2-2)
(added) libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp (+51)

diff --git a/libcxx/include/__mdspan/extents.h b/libcxx/include/__mdspan/extents.h
index 95082ef3d11ac9..54577f2c581dff 100644
--- a/libcxx/include/__mdspan/extents.h
+++ b/libcxx/include/__mdspan/extents.h
@@ -19,6 +19,7 @@
 
 #include <__assert>
 #include <__config>
+#include <__concepts/arithmetic.h>
 #include <__type_traits/common_type.h>
 #include <__type_traits/is_convertible.h>
 #include <__type_traits/is_nothrow_constructible.h>
@@ -282,8 +283,7 @@ class extents {
   using size_type  = make_unsigned_t<index_type>;
   using rank_type  = size_t;
 
-  static_assert(is_integral<index_type>::value && !is_same<index_type, bool>::value,
-                "extents::index_type must be a signed or unsigned integer type");
+  static_assert(__libcpp_integer<index_type>, "extents::index_type must be a signed or unsigned integer type");
   static_assert(((__mdspan_detail::__is_representable_as<index_type>(_Extents) || (_Extents == dynamic_extent)) && ...),
                 "extents ctor: arguments must be representable as index_type and nonnegative");
 
diff --git a/libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp b/libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp
new file mode 100644
index 00000000000000..ef28c456925df8
--- /dev/null
+++ b/libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp
@@ -0,0 +1,51 @@
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// <mdspan>
+
+// template<class IndexType, size_t... Extents>
+//  class extents;
+//
+// Mandates:
+//   - IndexType is a signed or unsigned integer type, and
+//   - each element of Extents is either equal to dynamic_extent, or is representable as a value of type IndexType.
+
+#include <cstddef>
+#include <climits>
+#include <mdspan>
+
+void invalid_index_types() {
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<bool, true> eb;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<char, '*'> ec;
+#ifndef TEST_HAS_NO_CHAR8_T
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<char8_t, u8'*'> ec8;
+#endif
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<char16_t, u'*'> ec16;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<char32_t, U'*'> ec32;
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<wchar_t, L'*'> ewc;
+#endif
+}
+
+void invalid_extent_values() {
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<signed char, static_cast<std::size_t>(SCHAR_MAX) + 1> esc1;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<signed char, std::dynamic_extent - 1> esc2;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<unsigned char, static_cast<std::size_t>(UCHAR_MAX) + 1> euc1;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<unsigned char, std::dynamic_extent - 1> euc2;
+}

github-actions · 2024-08-23T13:49:26Z

✅ With the latest revision this PR passed the C/C++ code formatter.

crtrott · 2024-08-23T14:27:48Z

Sorry I lost track of this. Its fine i don't need to do the work if soneone else takes the initiative ;-). I can review though.

PR 78086 (bddd8f4) provided the IMO exact approach - `__libcpp_integer` - for this.

crtrott

Looks fine too me, I do not understand why on some builds the verification test fails. The expected and observed message seem to me to match, and this seems to look like what we did in other places?

philnik777 · 2024-08-24T09:32:48Z

libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp

+
+void invalid_index_types() {
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents::index_type must be a signed or unsigned integer type}}
+  [[maybe_unused]] std::extents<char, '*'> ec;


I think I'd just add the appropriate -Wno-whatever flag to avoid the [[maybe_unused]]. They are quite noisy IMO.

I'd still like to see this addressed.

Sorry, I might have merged a bit too hastily. @frederick-vs-ja do you think you can make a follow-up PR?

libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp

philnik777 · 2024-08-24T09:35:11Z

libcxx/test/std/containers/views/mdspan/extents/index_type.verify.cpp

+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<signed char, static_cast<std::size_t>(SCHAR_MAX) + 1> esc1;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<signed char, std::dynamic_extent - 1> esc2;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<unsigned char, static_cast<std::size_t>(UCHAR_MAX) + 1> euc1;
+  // expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}
+  [[maybe_unused]] std::extents<unsigned char, std::dynamic_extent - 1> euc2;


Suggested change

// expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}

[[maybe_unused]] std::extents<signed char, static_cast<std::size_t>(SCHAR_MAX) + 1> esc1;

// expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}

[[maybe_unused]] std::extents<signed char, std::dynamic_extent - 1> esc2;

// expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}

[[maybe_unused]] std::extents<unsigned char, static_cast<std::size_t>(UCHAR_MAX) + 1> euc1;

// expected-error-re@*:* {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}

[[maybe_unused]] std::extents<unsigned char, std::dynamic_extent - 1> euc2;

// expected-error-re@*:* 4 {{static assertion failed {{.*}}extents ctor: arguments must be representable as index_type and nonnegative}}

[[maybe_unused]] std::extents<signed char, static_cast<std::size_t>(SCHAR_MAX) + 1> esc1;

[[maybe_unused]] std::extents<signed char, std::dynamic_extent - 1> esc2;

[[maybe_unused]] std::extents<unsigned char, static_cast<std::size_t>(UCHAR_MAX) + 1> euc1;

[[maybe_unused]] std::extents<unsigned char, std::dynamic_extent - 1> euc2;

I don't know whether this is actually better. Just a thought.

… tests

frederick-vs-ja requested a review from a team as a code owner August 23, 2024 13:45

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 23, 2024

frederick-vs-ja force-pushed the extents-mandates branch from 9cf646f to c098d0d Compare August 23, 2024 13:52

This comment was marked as resolved.

Sign in to view

[libc++] Disallow character types being index types of extents

e33f05f

PR 78086 (bddd8f4) provided the IMO exact approach - `__libcpp_integer` - for this.

frederick-vs-ja force-pushed the extents-mandates branch from c098d0d to e33f05f Compare August 23, 2024 14:36

crtrott approved these changes Aug 23, 2024

View reviewed changes

philnik777 reviewed Aug 24, 2024

View reviewed changes

frederick-vs-ja added 6 commits August 25, 2024 23:20

Merge branch 'main' into extents-mandates

ce597c3

Try to avoid regex in the expected-error

6ffb69d

Replace char with signed char in libcxx/containers/views/mdspan…

2eaa12c

… tests

Clang-format assert.ctor_from_array.pass.cpp

e998394

Merge branch 'main' into extents-mandates

0db680a

Fix assert.size.pass.cpp

647aa43

ldionne approved these changes Aug 27, 2024

View reviewed changes

ldionne merged commit 74e70ba into llvm:main Aug 27, 2024
55 checks passed

frederick-vs-ja deleted the extents-mandates branch August 28, 2024 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] Disallow character types being index types of `extents` #105832

[libc++] Disallow character types being index types of `extents` #105832

Uh oh!

frederick-vs-ja commented Aug 23, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 23, 2024

Uh oh!

github-actions bot commented Aug 23, 2024 •

edited

Loading

Uh oh!

This comment was marked as resolved.

crtrott commented Aug 23, 2024

Uh oh!

crtrott left a comment

Uh oh!

philnik777 Aug 24, 2024

Uh oh!

philnik777 Aug 27, 2024

Uh oh!

ldionne Aug 27, 2024

Uh oh!

Uh oh!

philnik777 Aug 24, 2024

Uh oh!

Uh oh!

Uh oh!

[libc++] Disallow character types being index types of extents #105832

[libc++] Disallow character types being index types of extents #105832

Uh oh!

Conversation

frederick-vs-ja commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 23, 2024

Uh oh!

github-actions bot commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

crtrott commented Aug 23, 2024

Uh oh!

crtrott left a comment

Choose a reason for hiding this comment

Uh oh!

philnik777 Aug 24, 2024

Choose a reason for hiding this comment

Uh oh!

philnik777 Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philnik777 Aug 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[libc++] Disallow character types being index types of `extents` #105832

[libc++] Disallow character types being index types of `extents` #105832

frederick-vs-ja commented Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 23, 2024 •

edited

Loading