Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions libcxx/include/__mdspan/extents.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

#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>
Expand Down Expand Up @@ -282,8 +284,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");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ int main(int, char**) {
}
// value out of range
{
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::extents<char, D, 5> e(arg); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::extents<signed char, D, 5> e(arg); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,13 @@ int main(int, char**) {
}
// value out of range
{
TEST_LIBCPP_ASSERT_FAILURE(
([] {
std::extents<char, D, 5> e1(std::array{1000, 5});
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
TEST_LIBCPP_ASSERT_FAILURE(([] { std::extents<signed char, D, 5> e1(std::array{1000, 5}); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// negative value
{
TEST_LIBCPP_ASSERT_FAILURE(
([] {
std::extents<char, D, 5> e1(std::array{-1, 5});
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
TEST_LIBCPP_ASSERT_FAILURE(([] { std::extents<signed char, D, 5> e1(std::array{-1, 5}); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
return 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ int main(int, char**) {
}
// value out of range
{
TEST_LIBCPP_ASSERT_FAILURE(([] { std::extents<char, D, 5> e1(1000, 5); }()),
TEST_LIBCPP_ASSERT_FAILURE(([] { std::extents<signed char, D, 5> e1(1000, 5); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// negative value
{
TEST_LIBCPP_ASSERT_FAILURE(([] { std::extents<char, D, 5> e1(-1, 5); }()),
TEST_LIBCPP_ASSERT_FAILURE(([] { std::extents<signed char, D, 5> e1(-1, 5); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ int main(int, char**) {
// value out of range
{
std::array args{1000, 5};
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::extents<char, D, 5> e1(std::span{args}); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::extents<signed char, D, 5> e1(std::span{args}); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// negative value
{
std::array args{-1, 5};
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::extents<char, D, 5> e1(std::span{args}); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::extents<signed char, D, 5> e1(std::span{args}); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,20 @@ int main(int, char**) {
}
// non-representability of extents itself
{
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_left::mapping<std::extents<char, D>> m(
std::layout_left::mapping<std::extents<int, D>>(std::extents<int, D>(500))); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::layout_left::mapping<std::extents<signed char, D>> m(
std::layout_left::mapping<std::extents<int, D>>(std::extents<int, D>(500)));
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// required_span_size not representable, while individual extents are
{
// check extents would be constructible
[[maybe_unused]] std::extents<char, D, 5> e(arg_exts);
[[maybe_unused]] std::extents<signed char, D, 5> e(arg_exts);
// but the product is not, so we can't use it for layout_left
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_left::mapping<std::extents<char, D, 5>> m(arg); }()),
([=] { std::layout_left::mapping<std::extents<signed char, D, 5>> m(arg); }()),
"layout_left::mapping converting ctor: other.required_span_size() must be representable as index_type.");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

// <mdspan>


// constexpr mapping(const extents_type& e) noexcept;
//
// Preconditions: The size of the multidimensional index space e is representable as a value of type index_type ([basic.fundamental]).
Expand All @@ -32,7 +31,7 @@ int main(int, char**) {
{
// the extents are representable but the product is not, so we can't use it for layout_left
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_left::mapping<std::extents<char, D, 5>> m(std::extents<char, D, 5>(100)); }()),
([=] { std::layout_left::mapping<std::extents<signed char, D, 5>> m(std::extents<signed char, D, 5>(100)); }()),
"layout_left::mapping extents ctor: product of extents must be representable as index_type.");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,12 @@ int main(int, char**) {
}
// non-representability of extents itself
{
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_left::mapping<std::extents<char, D>> m(
std::layout_right::mapping<std::extents<int, D>>(std::extents<int, D>(500))); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::layout_left::mapping<std::extents<signed char, D>> m(
std::layout_right::mapping<std::extents<int, D>>(std::extents<int, D>(500)));
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}

// Can't trigger required_span_size() representability assertion, since for rank-1 the above check will trigger first,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ int main(int, char**) {
// non-representability of extents itself
{
std::layout_stride::mapping arg(std::extents<int, D>(500), std::array<int, 1>{1});
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_left::mapping<std::extents<char, D>> m(arg); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_left::mapping<std::extents<signed char, D>> m(arg); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// non-representability of required span size
{
std::layout_stride::mapping arg(std::extents<int, D, D>(100, 3), std::array<int, 2>{1, 100});
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_left::mapping<std::extents<char, D, D>> m(arg); }()),
([=] { std::layout_left::mapping<std::extents<signed char, D, D>> m(arg); }()),
"layout_left::mapping from layout_stride ctor: other.required_span_size() must be "
"representable as index_type.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,20 @@ int main(int, char**) {
}
// non-representability of extents itself
{
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_right::mapping<std::extents<char, D>> m(
std::layout_right::mapping<std::extents<int, D>>(std::extents<int, D>(500))); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::layout_right::mapping<std::extents<signed char, D>> m(
std::layout_right::mapping<std::extents<int, D>>(std::extents<int, D>(500)));
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// required_span_size not representable, while individual extents are
{
// check extents would be constructible
[[maybe_unused]] std::extents<char, D, 5> e(arg_exts);
[[maybe_unused]] std::extents<signed char, D, 5> e(arg_exts);
// but the product is not, so we can't use it for layout_right
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_right::mapping<std::extents<char, D, 5>> m(arg); }()),
([=] { std::layout_right::mapping<std::extents<signed char, D, 5>> m(arg); }()),
"layout_right::mapping converting ctor: other.required_span_size() must be representable as index_type.");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ int main(int, char**) {
{
// the extents are representable but the product is not, so we can't use it for layout_right
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_right::mapping<std::extents<char, D, 5>> m(std::extents<char, D, 5>(100)); }()),
([=] {
std::layout_right::mapping<std::extents<signed char, D, 5>> m(std::extents<signed char, D, 5>(100));
}()),
"layout_right::mapping extents ctor: product of extents must be representable as index_type.");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,12 @@ int main(int, char**) {
}
// non-representability of extents itself
{
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_right::mapping<std::extents<char, D>> m(
std::layout_left::mapping<std::extents<int, D>>(std::extents<int, D>(500))); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::layout_right::mapping<std::extents<signed char, D>> m(
std::layout_left::mapping<std::extents<int, D>>(std::extents<int, D>(500)));
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}

// Can't trigger required_span_size() representability assertion, since for rank-1 the above check will trigger first,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ int main(int, char**) {
// non-representability of extents itself
{
std::layout_stride::mapping arg(std::extents<int, D>(500), std::array<int, 1>{1});
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_right::mapping<std::extents<char, D>> m(arg); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_right::mapping<std::extents<signed char, D>> m(arg); }()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// non-representability of required span size
{
std::layout_stride::mapping arg(std::extents<int, D, D>(100, 3), std::array<int, 2>{3, 1});
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_right::mapping<std::extents<char, D, D>> m(arg); }()),
([=] { std::layout_right::mapping<std::extents<signed char, D, D>> m(arg); }()),
"layout_right::mapping from layout_stride ctor: other.required_span_size() must be "
"representable as index_type.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,26 +65,26 @@ int main(int, char**) {
{
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::layout_stride::mapping<std::extents<char, D>> m(
std::layout_stride::mapping<std::extents<signed char, D>> m(
std::layout_stride::mapping<std::extents<int, D>>(std::extents<int, D>(500), std::array<int, 1>{1}));
}()),
"extents ctor: arguments must be representable as index_type and nonnegative");
}
// all strides must be larger than zero
{
always_convertible_layout::mapping<std::dextents<int, 2>> offset_map(std::dextents<int, 2>{10, 10}, 100, -1);
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_stride::mapping<std::extents<char, D, D>> m(offset_map); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_stride::mapping<std::extents<signed char, D, D>> m(offset_map); }()),
"layout_stride::mapping converting ctor: all strides must be greater than 0");
}
// required_span_size not representable, while individual extents are
{
std::extents<int, D, D> arg_exts{100, 5};
std::layout_stride::mapping<std::extents<int, D, D>> arg(arg_exts, std::array<int, 2>{1, 100});
// check extents would be constructible
[[maybe_unused]] std::extents<char, D, 5> e(arg_exts);
[[maybe_unused]] std::extents<signed char, D, 5> e(arg_exts);
// but the product is not, so we can't use it for layout_stride
TEST_LIBCPP_ASSERT_FAILURE(
([=] { std::layout_stride::mapping<std::extents<char, D, 5>> m(arg); }()),
([=] { std::layout_stride::mapping<std::extents<signed char, D, 5>> m(arg); }()),
"layout_stride::mapping converting ctor: other.required_span_size() must be representable as index_type.");
}
// required_span_size not representable, while individual extents are, edge case
Expand All @@ -104,7 +104,7 @@ int main(int, char**) {
// base offset must be 0 (i.e. mapping(0,...,0)==0) for a strided layout with positive strides
{
always_convertible_layout::mapping<std::dextents<int, 2>> offset_map(std::dextents<int, 2>{10, 10}, 3);
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_stride::mapping<std::extents<char, D, D>> m(offset_map); }()),
TEST_LIBCPP_ASSERT_FAILURE(([=] { std::layout_stride::mapping<std::extents<signed char, D, D>> m(offset_map); }()),
"layout_stride::mapping converting ctor: base offset of mapping must be zero.");
}
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ int main(int, char**) {
// the extents are representable but the product with strides is not, so we can't use it for layout_stride
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::layout_stride::mapping<std::extents<char, D, 5>> m(
std::extents<char, D, 5>(20), std::array<int, 2>{20, 1});
std::layout_stride::mapping<std::extents<signed char, D, 5>> m(
std::extents<signed char, D, 5>(20), std::array<int, 2>{20, 1});
}()),
"layout_stride::mapping ctor: required span size is not representable as index_type.");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ int main(int, char**) {
TEST_LIBCPP_ASSERT_FAILURE(
([=] {
std::array<int, 2> strides{20, 1};
std::layout_stride::mapping<std::extents<char, D, 5>> m(std::extents<char, D, 5>(20), std::span(strides));
std::layout_stride::mapping<std::extents<signed char, D, 5>> m(
std::extents<signed char, D, 5>(20), std::span(strides));
}()),
"layout_stride::mapping ctor: required span size is not representable as index_type.");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,18 @@ int main(int, char**) {
std::array<float, 10> data;
// make sure we are not failing because of using index_type instead of size_type
{
layout_wrapping_integral<4>::mapping<std::dextents<char, 2>> map(
std::dextents<char, 2>(100, 2), not_extents_constructible_tag());
std::mdspan<float, std::dextents<char, 2>, layout_wrapping_integral<4>> mds(data.data(), map);
assert(map.required_span_size() == char(8));
layout_wrapping_integral<4>::mapping<std::dextents<signed char, 2>> map(
std::dextents<signed char, 2>(100, 2), not_extents_constructible_tag());
std::mdspan<float, std::dextents<signed char, 2>, layout_wrapping_integral<4>> mds(data.data(), map);
assert(map.required_span_size() == static_cast<signed char>(8));
assert((static_cast<unsigned char>(200) == mds.size()));
}
{
layout_wrapping_integral<4>::mapping<std::dextents<char, 2>> map(
std::dextents<char, 2>(100, 3), not_extents_constructible_tag());
std::mdspan<float, std::dextents<char, 2>, layout_wrapping_integral<4>> mds(data.data(), map);
layout_wrapping_integral<4>::mapping<std::dextents<signed char, 2>> map(
std::dextents<signed char, 2>(100, 3), not_extents_constructible_tag());
std::mdspan<float, std::dextents<signed char, 2>, layout_wrapping_integral<4>> mds(data.data(), map);
// sanity check
assert(map.required_span_size() == char(12));
assert(map.required_span_size() == static_cast<signed char>(12));
// 100 x 3 exceeds 256
{
TEST_LIBCPP_ASSERT_FAILURE(([=] { mds.size(); }()), "mdspan: size() is not representable as size_type");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
//
// 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@*:* {{static assertion failed: extents::index_type must be a signed or unsigned integer type}}
[[maybe_unused]] std::extents<char, '*'> ec;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to see this addressed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#ifndef TEST_HAS_NO_CHAR8_T
// expected-error@*:* {{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@*:* {{static assertion failed: extents::index_type must be a signed or unsigned integer type}}
[[maybe_unused]] std::extents<char16_t, u'*'> ec16;
// expected-error@*:* {{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@*:* {{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;
Comment on lines +41 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Loading