Skip to content

Fix nd_span iterators to respect strides of non-contiguous views#3

Merged
pingelit merged 5 commits intomainfrom
copilot/fix-nd-span-iterator-strides
Feb 24, 2026
Merged

Fix nd_span iterators to respect strides of non-contiguous views#3
pingelit merged 5 commits intomainfrom
copilot/fix-nd-span-iterator-strides

Conversation

Copy link
Contributor

Copilot AI commented Feb 21, 2026

nd_span::begin()/end() returned raw pointers, iterating linearly over raw memory regardless of strides. This made range-based for loops and STL algorithms produce wrong results for subspans and transposed views (any view with non-unit or non-contiguous strides).

Changes

include/nd_array/nd_array.hpp

  • Added detail::nd_iterator<ElementType, MaxRank> stride-aware random access iterator template in the detail namespace, tracking a flat linear index and computing element addresses as data + Σ(indices[i] × strides[i])
  • Template parameter is the element type directly (Ty for mutable, const Ty for const) rather than a bool IsConst flag, making the iterator reusable by other classes in the same header
  • iterator_category is std::random_access_iterator_tag; full random access interface: operator-- (pre/post), operator+=/-=, operator+/-, operator[], operator</>/<=/>=, and iterator difference operator-
  • Supports implicit conversion from iterator to const_iterator
  • Handles edge cases: zero rank, zero-sized extents
  • Added iterator / const_iterator type aliases in nd_span pointing to detail::nd_iterator
  • Replaced raw-pointer begin()/end()/cbegin()/cend() with stride-aware iterator versions; end()/cend() pass size() so the end iterator has a well-defined flat index for arithmetic
  • Added #include <iterator> for iterator support types

tests/test_nd_span.cpp

  • Added file-scope static_assert checks verifying that nd_span<int>::iterator and nd_span<int>::const_iterator have iterator_category == std::random_access_iterator_tag
  • Added "nd_span - Stride-aware iteration" test case covering:
    • Column subspan (non-unit stride in outer dimension)
    • Transposed view (swapped strides)
    • Write-through mutation only touches elements inside the subspan
    • Random access operations on a contiguous span (operator[], operator+/-, operator+=/-=, decrement, iterator difference, ordering)
    • Random access arithmetic over a non-contiguous subspan

Example

// 3×5 matrix, values 0..14
nd_span<int> span(data.data(), 3, 5);

// Subspan of columns 1–3: extents [3,3], strides [5,1]
auto sub = span.subspan(1, {1, 4});

// Previously: iterated over raw memory [data+0 .. data+9] — wrong values/elements
// Now: visits span(0,1), span(0,2), span(0,3), span(1,1), ... correctly
for (auto v : sub) { /* stride-aware */ }

// Random access also works correctly
auto it = sub.begin();
it += 3;          // jump 3 elements forward
auto dist = sub.end() - sub.begin();  // == sub.size()
Original prompt

This section details on the original issue you should resolve

<issue_title>nd_span iterators do not consider the strides of subspans</issue_title>
<issue_description>As the title states. When using begin or end in a nd_span, the stride of that span is not considered and it iterates linearly over the contained memory. This is not useful when creating subspans from nd_array of other nd_spans.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@codecov
Copy link

codecov bot commented Feb 21, 2026

Codecov Report

❌ Patch coverage is 98.34254% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.97%. Comparing base (d3e9064) to head (c5bf0a1).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
include/nd_array/nd_array.hpp 96.55% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #3      +/-   ##
==========================================
+ Coverage   97.92%   97.97%   +0.04%     
==========================================
  Files           3        3              
  Lines        1206     1382     +176     
==========================================
+ Hits         1181     1354     +173     
- Misses         25       28       +3     
Flag Coverage Δ
Windows-windows-debug 97.70% <98.07%> (+0.04%) ⬆️
Windows-windows-msvc-debug 97.96% <98.34%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI and others added 2 commits February 21, 2026 12:29
Co-authored-by: pingelit <22936321+pingelit@users.noreply.github.com>
Co-authored-by: pingelit <22936321+pingelit@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix nd_span iterators to consider strides of subspans Fix nd_span iterators to respect strides of non-contiguous views Feb 21, 2026
Copilot AI requested a review from pingelit February 21, 2026 12:30
Copy link
Owner

@pingelit pingelit left a comment

Choose a reason for hiding this comment

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

This looks good, some minor changes to the iterator class would be nice to encourage reuse.

…instead of bool IsConst

Co-authored-by: pingelit <22936321+pingelit@users.noreply.github.com>
Copy link
Owner

@pingelit pingelit left a comment

Choose a reason for hiding this comment

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

I think it would be best if the iterator were random access or at least bidirectional. Also, test that the iterator is correctly implemented through a static_assert.

…gory tests

Co-authored-by: pingelit <22936321+pingelit@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

I think it would be best if the iterator were random access or at least bidirectional. Also, test that the iterator is correctly implemented through a static_assert.

Done in c5bf0a1. nd_iterator is now a full random access iterator (std::random_access_iterator_tag) with operator--, operator+=/−=, operator+/−, operator[], operator</>/<=/>=, and iterator difference operator−. The iterator tracks a flat linear index internally so all arithmetic is O(1). Two static_asserts at file scope in the test file verify the iterator category at compile time, and new runtime sections exercise decrement, arithmetic, difference, and ordering.

Copy link
Owner

@pingelit pingelit left a comment

Choose a reason for hiding this comment

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

Great, this address all my ideas.

@pingelit pingelit marked this pull request as ready for review February 24, 2026 20:50
@pingelit pingelit merged commit 35a30fe into main Feb 24, 2026
8 checks passed
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.

nd_span iterators do not consider the strides of subspans

2 participants