From db608052549a17415ee1e0406796179f36c5ec6c Mon Sep 17 00:00:00 2001 From: danieltowner Date: Tue, 6 Oct 2026 11:57:57 +0100 Subject: [PATCH] Fixed generator value parameter to match standard (#15) The generator constructor was passing a `size_t` as a parameter, not an `integral_constant` of `simd_size_t`. Added more tests to expose the issue and then fix it. Also re-enabled the `no-sign-compare` warning which was strongly related to misuse of the index, and fixed one extra error that occurred as a consequence. --- include/xvec/detail/core.hpp | 3 ++- include/xvec/detail/mask.hpp | 2 +- include/xvec/detail/utilities.hpp | 2 +- include/xvec/generic/generic_impl.hpp | 23 +++++++++++++---------- test/CMakeLists.txt | 1 - test/SimdTestUtilities.hpp | 2 +- test/TestSimdBit.cpp | 4 +++- test/TestSimdConstructor.cpp | 24 ++++++++++++++++++++++++ test/TestSimdMask.cpp | 12 ++++++++++++ 9 files changed, 57 insertions(+), 16 deletions(-) diff --git a/include/xvec/detail/core.hpp b/include/xvec/detail/core.hpp index 348e958..ebba3f8 100644 --- a/include/xvec/detail/core.hpp +++ b/include/xvec/detail/core.hpp @@ -314,7 +314,8 @@ concept generated_value_convertible_to = template concept generator_invocable_at = requires(_Gen __gen) { - { __gen(_Idx) } -> generated_value_convertible_to<_Tp>; + { __gen(std::integral_constant{}) } + -> generated_value_convertible_to<_Tp>; }; template diff --git a/include/xvec/detail/mask.hpp b/include/xvec/detail/mask.hpp index fad5156..39955d1 100644 --- a/include/xvec/detail/mask.hpp +++ b/include/xvec/detail/mask.hpp @@ -132,7 +132,7 @@ class basic_mask /// construct pre-computed mask values. /// @ingroup simd_mask_constructor /// @param fn The generator function to compute each bit. - constexpr basic_mask(std::invocable auto fn) : basic_mask(detail::generate_mask(target, fn)) {} + constexpr basic_mask(std::invocable> auto fn) : basic_mask(detail::generate_mask(target, fn)) {} /// Generate a mask which represents the bottom N bits of the mask. /// @internal diff --git a/include/xvec/detail/utilities.hpp b/include/xvec/detail/utilities.hpp index 3d19bb7..848ac34 100644 --- a/include/xvec/detail/utilities.hpp +++ b/include/xvec/detail/utilities.hpp @@ -249,7 +249,7 @@ constexpr void checkStaticMemoryBounds([[maybe_unused]] const std::string& op_na { #if defined(XVEC_ALWAYS_RANGE_CHECK_MEMORY) auto biggestIndex = reduce_max(indexes, m); - if (biggestIndex >= range_size) + if (std::size_t(biggestIndex) >= range_size) { std::string msg = "xvec range error for " + op_name + ". Range size:" + std::to_string(range_size) + diff --git a/include/xvec/generic/generic_impl.hpp b/include/xvec/generic/generic_impl.hpp index e06bc3c..c1465f7 100644 --- a/include/xvec/generic/generic_impl.hpp +++ b/include/xvec/generic/generic_impl.hpp @@ -55,15 +55,18 @@ constexpr auto generate(_Gp generator) { if constexpr (complex_number) { using _Tp = typename _Vp::traits::element_type; - return _Vp([=](std::index_sequence<_Idx...>) { - return typename _Vp::traits::builtin_type{_Tp((_Idx % 2) ? generator(_Idx / 2).imag() : generator(_Idx / 2).real())...}; - }(std::make_index_sequence<_Vp::size() * 2>())); + return _Vp([=](std::integer_sequence) { + return typename _Vp::traits::builtin_type{_Tp((_Idx % 2) ? + generator(size_constant<_Idx / 2>()).imag() : + generator(size_constant<_Idx / 2>()).real())...}; + }(std::make_integer_sequence())); } else { - return _Vp([=](std::index_sequence<_Idx...>) { - return typename _Vp::traits::builtin_type{std::bit_cast(generator(_Idx))...}; - }(std::make_index_sequence<_Vp::size()>())); + return _Vp([=](std::integer_sequence) { + return typename _Vp::traits::builtin_type{std::bit_cast( + generator(size_constant<_Idx>{}))...}; + }(std::make_integer_sequence())); } } @@ -151,18 +154,18 @@ constexpr _Mp generate_mask(generic_tag, _Gp generator) { using _Tp = container_for_num_bytes>; using _Vp = typename _Mp::builtin_type; - auto r = [=](std::index_sequence<_Idx...>) { + auto r = [=](std::integer_sequence) { return _Vp{_Tp(generator(size_constant<_Idx>()) ? ~_Tp() : _Tp())...}; - }(std::make_index_sequence<_Mp::size()>()); + }(std::make_integer_sequence()); return _Mp::from_builtin(r); } template constexpr _Mp generate_mask(compact_mask_tag, _Gp generator) { - return _Mp::from_builtin([=](std::index_sequence<_Idx...>) { + return _Mp::from_builtin([=](std::integer_sequence) { return ((typename _Mp::builtin_type(generator(size_constant<_Idx>{})) << _Idx) | ...); - } (std::make_index_sequence<_Mp::size>())); + } (std::make_integer_sequence())); } ///@} diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 8df58e9..584202e 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -236,7 +236,6 @@ target_compile_options( -Wextra -Werror -Wno-psabi - -Wno-sign-compare "${XVEC_TEST_OLEVEL}" -fno-fast-math ) diff --git a/test/SimdTestUtilities.hpp b/test/SimdTestUtilities.hpp index caaa2d4..55c0a69 100644 --- a/test/SimdTestUtilities.hpp +++ b/test/SimdTestUtilities.hpp @@ -131,7 +131,7 @@ GetConstexprRandomVector (int limit = 32) limit = limit * 2; // Hashy-like algorithm to pick a number. Relies on the position and the incoming `seed` index. - auto genRnd = [=](auto i) -> typename _V::value_type + auto genRnd = [=](xvec::simd::simd_size_type i) -> typename _V::value_type { auto v = (r[(i + 7) & 0xF] ^ std::rotl(r[(i + index) & 0xF], i)) % int(limit); if (hasNegative && ((v & 1) == 1)) diff --git a/test/TestSimdBit.cpp b/test/TestSimdBit.cpp index 6035799..66942de 100644 --- a/test/TestSimdBit.cpp +++ b/test/TestSimdBit.cpp @@ -347,7 +347,9 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(BitReverse, TypeParam, UnsignedSimdTypes) #if defined(_XVEC_HAS_CONSTEXPR) { constexpr auto cv = GetConstexprRandomVector(); - static_assert(to_array(bit_reverse(cv)) == applyUnaryToArray(cv, expected_bit_reverse)); + constexpr auto reversed = bit_reverse(cv); + constexpr auto expected = applyUnaryToArray(cv, expected_bit_reverse); + static_assert(to_array(reversed) == expected); } #endif } diff --git a/test/TestSimdConstructor.cpp b/test/TestSimdConstructor.cpp index 6de52ca..444f260 100644 --- a/test/TestSimdConstructor.cpp +++ b/test/TestSimdConstructor.cpp @@ -275,6 +275,30 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(Generator, TypeParam, AllSimdTypes) BOOST_TEST(to_array(computedConstexpr) == expected, boost::test_tools::per_element()); } +BOOST_AUTO_TEST_CASE(GeneratorIndexType) +{ + using Vec = xvec::simd::vec; + using ComplexVec = xvec::simd::vec, 4>; + using xvec::simd::simd_size_type; + + // Check that the generator passes a compile-time constant of the correct type. + const Vec computed([](std::integral_constant) -> int { + return I + 2; + }); + const std::array expected{2, 3, 4, 5}; + BOOST_TEST(to_array(computed) == expected, boost::test_tools::per_element()); + + // Same again, but for complex (which currently uses a different code path). + const ComplexVec complexComputed([]( + std::integral_constant) -> std::complex { + return {float(I + 2), float(I + 12)}; + }); + const std::array, 4> complexExpected{{ + {2.0f, 12.0f}, {3.0f, 13.0f}, {4.0f, 14.0f}, {5.0f, 15.0f} + }}; + BOOST_TEST(to_array(complexComputed) == complexExpected, boost::test_tools::per_element()); +} + void CheckDisallowedGenerators() { struct S { diff --git a/test/TestSimdMask.cpp b/test/TestSimdMask.cpp index 4089377..c687301 100644 --- a/test/TestSimdMask.cpp +++ b/test/TestSimdMask.cpp @@ -358,6 +358,18 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(InitialiseFromGenerator, TypeParam, AllSimdMaskTyp #endif } +BOOST_AUTO_TEST_CASE(GeneratorIndexType) +{ + using Mask = xvec::simd::mask; + using xvec::simd::simd_size_type; + + // Only accepts integral_constant, not a plain index. + const Mask computed([](std::integral_constant) -> bool + { return I == 0 || I == 3; }); + + BOOST_TEST(computed.to_bitset() == std::bitset<4>(0b1001)); +} + BOOST_AUTO_TEST_CASE_TEMPLATE(InitialiseNbitMask, TypeParam, AllSimdMaskTypes) { using xvec::simd::mask_from_count;