Skip to content

Fix mem concepts non-copyable pitch test#6

Closed
psychocoderHPC wants to merge 1 commit intodevfrom
pr-fix-mem-concepts-non-copyable-pitch
Closed

Fix mem concepts non-copyable pitch test#6
psychocoderHPC wants to merge 1 commit intodevfrom
pr-fix-mem-concepts-non-copyable-pitch

Conversation

@psychocoderHPC
Copy link
Copy Markdown
Owner

@psychocoderHPC psychocoderHPC commented Apr 8, 2026

Split from local commit 7cba5e0.

Base branch: dev.

Summary by CodeRabbit

  • Tests
    • Updated regression test for multi-dimensional span pitch computation to correctly handle non-copyable element types.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

📝 Walkthrough

Walkthrough

A regression test for alpaka::concepts::IDataSource/IMdSpan access return type was corrected to compute pitches using alpaka::calculatePitchesFromExtents<NonCopyStruct>() instead of <int>, ensuring pitch computation matches the element type used for MdSpan construction. The test comment was updated to document this non-copyable element-type scenario.

Changes

Cohort / File(s) Summary
Test Regression Fix
test/unit/mem/concepts.cpp
Updated pitch calculation to use NonCopyStruct element type instead of int, aligning with the actual MdSpan element type. Comment clarified the non-copyable element-type scenario being tested.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A pitch once computed with the wrong type in mind,
Now matches the struct that MdSpan shall find,
With NonCopyStruct aligned from start to the end,
Our test consistency grows, hop hop, my friend! 🐇

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix mem concepts non-copyable pitch test' directly and clearly describes the main change: fixing a test related to non-copyable pitch computation in the mem concepts module.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr-fix-mem-concepts-non-copyable-pitch

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/unit/mem/concepts.cpp (1)

193-193: Refactor: use correct element type for pitch calculation.

This line has the same issue that was fixed on line 228: the pitches are computed from sizeof(int) but used to construct MdSpan objects with float and float const element types (lines 195, 197). While sizeof(int) == sizeof(float) on most platforms, this is semantically incorrect and could produce wrong results on unusual architectures.

♻️ Proposed fix
-    constexpr concepts::Vector auto pitches = alpaka::calculatePitchesFromExtents<int>(extents);
+    constexpr concepts::Vector auto pitches = alpaka::calculatePitchesFromExtents<float>(extents);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/unit/mem/concepts.cpp` at line 193, The pitches are being calculated
with alpaka::calculatePitchesFromExtents<int>(extents) but later used to
construct MdSpan objects with float/float const element types; change the
pitches calculation to use the correct element type (e.g.,
alpaka::calculatePitchesFromExtents<float>(extents) or the appropriate element
type of the MdSpan) so the pitch computation matches the MdSpan element size
(adjust any related constexpr concepts::Vector declaration accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/unit/mem/concepts.cpp`:
- Line 193: The pitches are being calculated with
alpaka::calculatePitchesFromExtents<int>(extents) but later used to construct
MdSpan objects with float/float const element types; change the pitches
calculation to use the correct element type (e.g.,
alpaka::calculatePitchesFromExtents<float>(extents) or the appropriate element
type of the MdSpan) so the pitch computation matches the MdSpan element size
(adjust any related constexpr concepts::Vector declaration accordingly).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d5ae9b63-6682-475c-ae01-bcf9bc5f8335

📥 Commits

Reviewing files that changed from the base of the PR and between 33568ee and 7cba5e0.

📒 Files selected for processing (1)
  • test/unit/mem/concepts.cpp

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.

1 participant