Skip to content

[Mirror] Implement canonicalize slices for mdspan (P3663) - #18

Open
csiefer2 wants to merge 3 commits into
developfrom
pr-mirror-9292
Open

[Mirror] Implement canonicalize slices for mdspan (P3663)#18
csiefer2 wants to merge 3 commits into
developfrom
pr-mirror-9292

Conversation

@csiefer2

Copy link
Copy Markdown
Owner

Automated mirror of upstream PR kokkos#9292 Testing to discover issues with the mdspan changes. This is the state after kokkos/mdspan#457

crtrott added 3 commits June 26, 2026 15:36
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces slice canonicalization, static bounds checking, and constant_wrapper utilities for submdspan, alongside namespace cleanups and precondition checks for nonnegative extents. The review feedback highlights a potential bug in is_nonnegative_and_representable where negative values wrapped in integral-constant-like types could bypass checks when compared against unsigned types. Additionally, compilation issues in C++14 mode were identified in layout_stride.hpp due to referencing padded mapping traits when layout_padded_fwd.hpp is not included, for which compatible helper traits have been suggested.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +225 to +246
template <class R, class T>
MDSPAN_INLINE_FUNCTION constexpr bool is_nonnegative_and_representable(T t) noexcept {
// T might not be integral and thus invalid to pass to in_range
// Only check this if we can actually call in_range
if constexpr (std::is_integral_v<T>)
{
if constexpr (std::is_signed_v<T>) {
if (t < 0)
return false;
}

return in_range<R>(t);
} else
{
if constexpr (std::is_signed_v<R>) {
if (static_cast<R>(t) < 0)
return false;
}

return true;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In is_nonnegative_and_representable, if T is an integral-constant-like type (such as std::integral_constant or constant_wrapper) wrapping a negative value, std::is_integral_v<T> evaluates to false. If R is an unsigned type (like size_t), the else branch will be taken, and since std::is_signed_v<R> is false, the function will incorrectly return true for negative values. This bypasses precondition checks and can lead to silent undefined behavior.

To fix this, we can define a helper to extract the underlying value of integral-constant-like types before performing the checks.

template <class T, class = void>
struct has_value_member : std::false_type {};

template <class T>
struct has_value_member<T, decltype((void)T::value, void())> : std::true_type {};

template <class T>
constexpr auto get_absolute_value(T t) {
  if constexpr (has_value_member<T>::value) {
    return T::value;
  } else {
    return t;
  }
}

template <class R, class T>
MDSPAN_INLINE_FUNCTION constexpr bool is_nonnegative_and_representable(T t) noexcept {
  auto val = get_absolute_value(t);
  using V = decltype(val);
  if constexpr (std::is_integral_v<V>)
  {
    if constexpr (std::is_signed_v<V>) {
      if (val < 0)
        return false;
    }

    return in_range<R>(val);
  } else
  {
    if constexpr (std::is_signed_v<R>) {
      if (static_cast<R>(val) < 0)
        return false;
    }

    return true;
  }
}

Comment on lines +24 to +26
#if MDSPAN_HAS_CXX_17
#include "../__p2642_bits/layout_padded_fwd.hpp"
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In C++14, layout_padded_fwd.hpp is not included because MDSPAN_HAS_CXX_17 is false. However, detail::is_layout_left_padded_mapping and detail::is_layout_right_padded_mapping are still referenced in MDSPAN_CONDITIONAL_EXPLICIT (which is compiled in C++14), leading to compilation failures. We can define helper traits that fall back to std::false_type in C++14 to maintain compatibility.

#if MDSPAN_HAS_CXX_17
#include "../__p2642_bits/layout_padded_fwd.hpp"
namespace MDSPAN_IMPL_STANDARD_NAMESPACE {
namespace detail {
template <class Mapping>
using is_layout_left_padded_mapping_or_false = is_layout_left_padded_mapping<Mapping>;
template <class Mapping>
using is_layout_right_padded_mapping_or_false = is_layout_right_padded_mapping<Mapping>;
}
}
#else
namespace MDSPAN_IMPL_STANDARD_NAMESPACE {
namespace detail {
template <class Mapping>
struct is_layout_left_padded_mapping_or_false : std::false_type {};
template <class Mapping>
struct is_layout_right_padded_mapping_or_false : std::false_type {};
}
}
#endif

Comment on lines +448 to +449
detail::is_layout_left_padded_mapping<StridedLayoutMapping>::value || // Don't need to guard for C++14 as this isn't compiled in < C++20
detail::is_layout_right_padded_mapping<StridedLayoutMapping>::value ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Use the C++14-compatible helper traits to avoid compilation errors when compiling in C++14 mode.

        detail::is_layout_left_padded_mapping_or_false<StridedLayoutMapping>::value ||
        detail::is_layout_right_padded_mapping_or_false<StridedLayoutMapping>::value ||

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants