Skip to content

Expand MdForwardIter host coverage#8

Closed
psychocoderHPC wants to merge 1 commit intodevfrom
pr-expand-mdforwarditer-host-coverage
Closed

Expand MdForwardIter host coverage#8
psychocoderHPC wants to merge 1 commit intodevfrom
pr-expand-mdforwarditer-host-coverage

Conversation

@psychocoderHPC
Copy link
Copy Markdown
Owner

@psychocoderHPC psychocoderHPC commented Apr 8, 2026

Split from local commit 6110d5c.

Base branch: dev.

Summary by CodeRabbit

  • Bug Fixes

    • Improved const-qualified iterator access for multi-dimensional spans to properly return const iterators.
    • Added const-qualified iterator support to multi-dimensional array containers.
  • Tests

    • Expanded test coverage for multi-dimensional array iteration including empty array handling, traversal order validation, const correctness enforcement, and iterator increment behavior.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 51b6511c-2fac-4e41-926d-4f73b4757a23

📥 Commits

Reviewing files that changed from the base of the PR and between 6110d5c and 8995315.

📒 Files selected for processing (3)
  • include/alpaka/mem/MdSpan.hpp
  • include/alpaka/mem/MdSpanArray.hpp
  • test/unit/mem/mdIterator.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • include/alpaka/mem/MdSpan.hpp
  • include/alpaka/mem/MdSpanArray.hpp

📝 Walkthrough

Walkthrough

This PR refines iterator access in MdSpan and MdSpanArray classes. The const-qualified begin()/end() methods now construct iterators from getConstMdSpan() instead of *this. New const overloads are added to MdSpanArray. Comprehensive unit tests validate iterator behavior across various container configurations and edge cases.

Changes

Cohort / File(s) Summary
Iterator const overload updates
include/alpaka/mem/MdSpan.hpp, include/alpaka/mem/MdSpanArray.hpp
Updated begin() const and end() const methods to construct iterators from getConstMdSpan() rather than *this. New const overloads added to MdSpanArray.
Iterator test suite expansion
test/unit/mem/mdIterator.cpp
Replaced single test with comprehensive Catch2 test suite. Added collectValues helper and multiple test sections covering zero extents, 1D/3D traversal order, mutable/const iteration semantics, and increment behaviors.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Hopping through spans so true,
Const iterators, fresh and new,
getConstMdSpan leads the way,
Through dimensions, hooray, hooray!
Tests validate each bound we take, 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: expanding test coverage for MdForwardIter (iterator access) on the host platform across MdSpan and MdSpanArray.

✏️ 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-expand-mdforwarditer-host-coverage

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

@psychocoderHPC psychocoderHPC changed the base branch from dev to pr-add-idxrange-host-iterator-coverage April 8, 2026 15:59
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/mdIterator.cpp (1)

92-105: Add direct const-iterator checks for MdSpan and MdSpanArray.

The new read-only assertions currently only prove the View path. Adding one const MdSpan and one const MdSpanArray round-trip here would exercise the overloads changed in include/alpaka/mem/MdSpan.hpp and include/alpaka/mem/MdSpanArray.hpp.

➕ Suggested coverage
         std::array<int, 6> storage{2, 4, 6, 8, 10, 12};
         auto view = alpaka::makeView(api::host, storage.data(), alpaka::Vec{2u, 3u});
         auto const constView = view.getConstView();
+        auto span = alpaka::makeMdSpan(storage.data(), alpaka::Vec{2u, 3u});
+        int arrayStorage[2][3]{{2, 4, 6}, {8, 10, 12}};
+        alpaka::MdSpanArray<decltype(arrayStorage), uint32_t> arraySpan(arrayStorage);
 
         static_assert(!std::is_const_v<std::remove_reference_t<decltype(*view.begin())>>);
         static_assert(std::is_const_v<std::remove_reference_t<decltype(*constView.begin())>>);
         static_assert(std::is_const_v<std::remove_reference_t<decltype(*std::as_const(view).begin())>>);
+        static_assert(std::is_const_v<std::remove_reference_t<decltype(*std::as_const(span).begin())>>);
+        static_assert(std::is_const_v<std::remove_reference_t<decltype(*std::as_const(arraySpan).begin())>>);
 
         REQUIRE(collectValues(view) == std::vector<int>{2, 4, 6, 8, 10, 12});
         REQUIRE(collectValues(constView) == collectValues(view));
+        REQUIRE(collectValues(std::as_const(span)) == collectValues(view));
+        REQUIRE(collectValues(std::as_const(arraySpan)) == collectValues(view));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/unit/mem/mdIterator.cpp` around lines 92 - 105, Add tests that exercise
the const-iterator overloads for MdSpan and MdSpanArray: create a mutable MdSpan
and MdSpanArray from the same storage used for View, obtain their const variants
(e.g., via getConstView-like API or by binding to const MdSpan/MdSpanArray), add
static_asserts that *const* iterators yield const references (similar to the
View checks) and REQUIRE that collectValues(constMdSpan) and
collectValues(constMdSpanArray) equal the mutable collectValues results; target
the types MdSpan and MdSpanArray and the helper collectValues to ensure the
MdSpan.hpp and MdSpanArray.hpp const overloads are exercised.
🤖 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/mdIterator.cpp`:
- Around line 92-105: Add tests that exercise the const-iterator overloads for
MdSpan and MdSpanArray: create a mutable MdSpan and MdSpanArray from the same
storage used for View, obtain their const variants (e.g., via getConstView-like
API or by binding to const MdSpan/MdSpanArray), add static_asserts that *const*
iterators yield const references (similar to the View checks) and REQUIRE that
collectValues(constMdSpan) and collectValues(constMdSpanArray) equal the mutable
collectValues results; target the types MdSpan and MdSpanArray and the helper
collectValues to ensure the MdSpan.hpp and MdSpanArray.hpp const overloads are
exercised.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5e7c9b73-9f14-491e-a025-c3dec5d07b04

📥 Commits

Reviewing files that changed from the base of the PR and between 33568ee and 6110d5c.

📒 Files selected for processing (5)
  • include/alpaka/mem/MdSpan.hpp
  • include/alpaka/mem/MdSpanArray.hpp
  • test/unit/mem/concepts.cpp
  • test/unit/mem/idxRange.cpp
  • test/unit/mem/mdIterator.cpp

@psychocoderHPC
Copy link
Copy Markdown
Owner Author

@coderabbitai review

@psychocoderHPC psychocoderHPC force-pushed the pr-expand-mdforwarditer-host-coverage branch from 19a8ada to 1588388 Compare April 8, 2026 18:05
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@psychocoderHPC psychocoderHPC force-pushed the pr-add-idxrange-host-iterator-coverage branch from cf3aac2 to 67eba11 Compare April 8, 2026 18:22
@psychocoderHPC psychocoderHPC force-pushed the pr-expand-mdforwarditer-host-coverage branch from 1588388 to 8995315 Compare April 8, 2026 18:24
@psychocoderHPC psychocoderHPC changed the base branch from pr-add-idxrange-host-iterator-coverage to dev April 8, 2026 18:24
@psychocoderHPC
Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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