Skip to content

Commit 81766e6

Browse files
committed
tighten(hpc/blocked_grid): apply P2 savant pre-merge findings + persist verdict
P2 savant (Phase 13) verdict: SHIP-WITH-FOLLOWUPS. 4 P2 findings; 3 applied in this commit, 1 deferred to PR-X3.1. P2-1 (applied) — downscope `pub` helpers on GridBlock/GridBlockMut to `pub(crate)`. The four helpers (data_slice, padded_cols_stride on GridBlock; data_mut, padded_cols on GridBlockMut) are intra-crate implementation seams. Leaving them `pub` meant downstream consumers could bypass the `# Footgun` guard on `as_padded_slice`. Also drops the `#[doc(hidden)]` attribute — no longer needed once visibility is tight. P2-3 (applied) — drop stray `T: Copy` bound from `GridBlock::from_grid`, `GridBlockMut::from_grid`, `Iterator for BaseBlockIter`, `Iterator for BaseBlockIterMut`, both `ExactSizeIterator` impls, and the impl block holding `blocks_base` / `blocks_base_mut`. None of these positions actually copy a `T` value — they only compute index arithmetic and slice the storage. The bound was over-constraining; iterator surface now works for any `T` (not just `T: Copy`). `BlockedGrid::get` / `set` still correctly require `T: Copy` because they do copy values. P2-4 (applied) — strengthen macro L1-only deferral wording with explicit PR-X3.1 ticket reference + `TODO(PR-X3.1)` marker + dedicated per-field workaround warning. Reduces the risk that callers cement per-field loops outside the macro-generated struct. P2-2 (DEFERRED → PR-X3.1) — typed `field_grid::<I, FieldT>()` accessor alongside the existing erased `field_n::<I>()`. Additive but requires either a downcast trait or extra macro emit arm; no current consumer needs it. Verdict file persisted at .claude/knowledge/pr-x3-p2-savant-review.md. PR-X3.1 follow-up backlog documented at the bottom of the verdict file. All 5 gates green after tightenings: - cargo check: PASS - cargo test --lib hpc::blocked_grid: 111/111 PASS - cargo test --doc hpc::blocked_grid: 79/79 PASS - cargo fmt --check: clean - cargo clippy -D warnings: clean
1 parent 01a70ed commit 81766e6

4 files changed

Lines changed: 90 additions & 35 deletions

File tree

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
# PR-X3 P2 Savant Pre-Merge Review — Verdict
2+
3+
Reviewer: Sonnet P2 savant (Phase 13 of PR-X3 sprint)
4+
PR: AdaWorldAPI/ndarray#158
5+
Branch reviewed: `claude/pr-x3-cognitive-grid-design` @ `01a70edb`
6+
7+
Verdict: **SHIP-WITH-FOLLOWUPS**
8+
9+
P2 count: 4 (3 applied pre-merge, 1 deferred to PR-X3.1)
10+
11+
## Highest-leverage tightenings (rank-ordered)
12+
13+
1. **P2-3 — drop stray `T: Copy` from `from_grid` + iterator impls** (APPLIED) — `src/hpc/blocked_grid/base.rs:334, 482` and `iter.rs:55, 87, 144, 187, 193`
14+
2. **P2-1 — downscope four helpers to `pub(crate)`** (APPLIED) — `src/hpc/blocked_grid/base.rs:413, 421, 557, 563`
15+
3. **P2-4 — macro deferral wording strengthened with PR-X3.1 marker** (APPLIED) — `src/hpc/blocked_grid/grid_struct_macro.rs:12-18`
16+
4. **P2-2 — typed `field_grid::<I, FieldT>()` accessor** (DEFERRED → PR-X3.1)
17+
18+
## Detailed findings + rulings
19+
20+
### P2-1: `pub` helpers on `GridBlock` / `GridBlockMut``pub(crate)` (APPLIED)
21+
22+
Worker A4 added `data_slice()` / `padded_cols_stride()` on `GridBlock` and `data_mut()` / `padded_cols()` on `GridBlockMut` as `#[doc(hidden)] pub` to enable sibling-module access. Leaving them `pub` means downstream consumers can call `blk.data_slice()` and bypass the `# Footgun` guard on `as_padded_slice`. Downscoped all four to `pub(crate)` and dropped the `#[doc(hidden)]` attribute (no longer needed once visibility is tightened).
23+
24+
### P2-2: `field_n::<I>()` type erasure (DEFERRED → PR-X3.1)
25+
26+
Returns `&dyn FieldGridRef` (erased type), matching the W3-W6 `soa_struct!` pattern. Adding a typed `field_grid::<I, FieldT>()` accessor is additive but requires either a `FromFieldGridRef` downcast trait or a macro-generated per-field method — neither is trivially additive without a new trait or extra macro emit arm. The erased form is sufficient for the PR-X3 use case (dimension parity checks); the typed accessor would unlock `let edge: &BlockedGrid<u64, 64, 64> = g.field_grid::<0, u64>()` but no current consumer needs it. Queue for PR-X3.1 alongside macro L2/L3/L4 deferral.
27+
28+
### P2-3: Stray `T: Copy` bound on iterator surface (APPLIED)
29+
30+
`GridBlock::from_grid` / `GridBlockMut::from_grid` carried `where T: Copy` even though their bodies only compute index arithmetic and slice `&grid.data[start..end]` — no `T` value is ever copied. This bound propagated into `Iterator for BaseBlockIter` / `BaseBlockIterMut` / `ExactSizeIterator` / the `impl<T: Copy> BlockedGrid<T, BR, BC>` block holding `blocks_base` / `blocks_base_mut`. A consumer with `BlockedGrid<MyNonCopyType, 8, 8>` could only `get` / `set` cells, not iterate. Removed the bound from all six sites. `BlockedGrid::get` / `set` still correctly require `T: Copy` (they actually copy values).
31+
32+
### P2-4: Macro L1-only deferral wording (APPLIED)
33+
34+
The v1 macro emits `map_l1` / `bulk_apply_l1` / `blocks_l1` on the generated struct; L2/L3/L4 are deferred. The deferral itself is the right call (emitting lockstep `{Name}L2Block` for a four-field struct requires `paste!`-generated types with `N=4` const generics — non-trivial without regression risk). But the v1 deferral note was low-visibility, risking callers cementing per-field workarounds. Strengthened the wording: explicit PR-X3.1 ticket reference + `TODO(PR-X3.1)` marker + dedicated "per-field workaround warning" subsection alerting readers that per-field call sites won't auto-migrate when PR-X3.1 lands.
35+
36+
## CI signal
37+
38+
No fragile tests in the new modules: no timing-dependent, no env-dependent, no `#[ignore]`-gated tests. The `BaseBlockIterMut` raw-pointer lending-iterator carries three `// SAFETY:` annotations accounting for the aliasing invariant — appropriate level of annotation for this pattern. No CI concern.
39+
40+
The `paste = "1"` dep (P1-2 from codex audit) is already in the workspace lock and has zero binary impact.
41+
42+
## Net call
43+
44+
Three P2 tightenings applied as a same-day follow-up commit on this branch. P2-2 (typed `field_grid` accessor) correctly post-merge — queued for PR-X3.1 alongside the macro L2/L3/L4 emission.
45+
46+
After this commit lands, PR #158 flips draft → ready-for-review and advances to the merge ladder.
47+
48+
## PR-X3.1 follow-up backlog
49+
50+
Queued for a small same-week follow-up PR:
51+
1. Emit lockstep `{Name}L{2,3,4}Block` block view types + `map_l{2,3,4}` + `bulk_apply_l{2,3,4}` methods on the macro-generated SoA-of-grids struct
52+
2. Add `field_grid::<I, FieldT>()` typed accessor alongside the existing `field_n::<I>()` erased accessor
53+
3. Naming consistency: rename `GridBlockMut::padded_cols``padded_cols_stride` to match `GridBlock::padded_cols_stride`

src/hpc/blocked_grid/base.rs

Lines changed: 19 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -331,10 +331,7 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlock<'a, T, BR, BC> {
331331
/// assert_eq!(blk.row_origin(), 4);
332332
/// assert_eq!(blk.col_origin(), 4);
333333
/// ```
334-
pub fn from_grid(grid: &'a BlockedGrid<T, BR, BC>, block_row: usize, block_col: usize) -> Self
335-
where
336-
T: Copy,
337-
{
334+
pub fn from_grid(grid: &'a BlockedGrid<T, BR, BC>, block_row: usize, block_col: usize) -> Self {
338335
let row_origin = block_row * BR;
339336
let col_origin = block_col * BC;
340337
let start = row_origin * grid.padded_cols + col_origin;
@@ -405,20 +402,16 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlock<'a, T, BR, BC> {
405402
self.col_origin
406403
}
407404

408-
/// Access internal data slice.
409-
///
410-
/// Used by compute.rs (worker A4) to implement `row()` / `rows()` on
411-
/// `GridBlock` without re-opening base.rs.
412-
#[doc(hidden)]
413-
pub fn data_slice(&self) -> &[T] {
405+
/// Access internal data slice (intra-crate seam used by `compute.rs` /
406+
/// `iter.rs` / `super_block.rs`). `pub(crate)` — not exposed downstream
407+
/// since callers should go through `GridBlock::row` / `row_mut`, which
408+
/// enforce the BR/BC bounds.
409+
pub(crate) fn data_slice(&self) -> &[T] {
414410
self.data
415411
}
416412

417-
/// Access padded_cols stride.
418-
///
419-
/// Used by compute.rs (worker A4) to implement `row()` on `GridBlock`.
420-
#[doc(hidden)]
421-
pub fn padded_cols_stride(&self) -> usize {
413+
/// Access padded_cols stride (intra-crate seam — see `data_slice` note).
414+
pub(crate) fn padded_cols_stride(&self) -> usize {
422415
self.padded_cols
423416
}
424417

@@ -479,10 +472,7 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
479472
/// assert_eq!(blk.block_row(), 1);
480473
/// assert_eq!(blk.row_origin(), 4);
481474
/// ```
482-
pub fn from_grid(grid: &'a mut BlockedGrid<T, BR, BC>, block_row: usize, block_col: usize) -> Self
483-
where
484-
T: Copy,
485-
{
475+
pub fn from_grid(grid: &'a mut BlockedGrid<T, BR, BC>, block_row: usize, block_col: usize) -> Self {
486476
let row_origin = block_row * BR;
487477
let col_origin = block_col * BC;
488478
let start = row_origin * grid.padded_cols + col_origin;
@@ -552,15 +542,19 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
552542
self.col_origin
553543
}
554544

555-
/// Access internal data slice (for use by future iterator workers).
556-
#[doc(hidden)]
557-
pub fn data_mut(&mut self) -> &mut [T] {
545+
/// Access internal mutable data slice (intra-crate seam used by `iter.rs`
546+
/// / `compute.rs`). `pub(crate)` — not exposed downstream since callers
547+
/// should go through `GridBlockMut::row_mut`, which enforces the BR
548+
/// bound.
549+
pub(crate) fn data_mut(&mut self) -> &mut [T] {
558550
self.data
559551
}
560552

561-
/// Access padded_cols stride (for use by future iterator workers).
562-
#[doc(hidden)]
563-
pub fn padded_cols(&self) -> usize {
553+
/// Access padded_cols stride (intra-crate seam — see `data_mut` note).
554+
/// NOTE: Named `padded_cols` (not `padded_cols_stride` as on `GridBlock`)
555+
/// for back-compat with `iter.rs`'s `row_mut` impl. Naming consistency is
556+
/// queued as PR-X3.1 housekeeping.
557+
pub(crate) fn padded_cols(&self) -> usize {
564558
self.padded_cols
565559
}
566560

src/hpc/blocked_grid/grid_struct_macro.rs

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,19 @@
1212
//! # Tier coverage (v1)
1313
//!
1414
//! **v1 emits L1 methods only.** L2/L3/L4 alias methods on the generated
15-
//! struct are deferred to a follow-up PR. The reserved field names below
16-
//! include L2-L4 identifiers so future emission won't break callers. Callers
17-
//! that need L2/L3/L4 can access individual fields and call `.blocks_l2()` /
18-
//! `.map_l2()` / `.bulk_apply_l2()` directly on each
19-
//! `BlockedGrid<FieldT, 64, 64>` field.
15+
//! struct are deferred to **PR-X3.1** (TODO(PR-X3.1): emit lockstep L2/L3/L4
16+
//! block view types `{Name}L{2,3,4}Block` and the corresponding `map_l*` /
17+
//! `bulk_apply_l*` methods). The reserved field names below include L2-L4
18+
//! identifiers so the future emission cannot break callers.
19+
//!
20+
//! **Per-field workaround warning.** Callers who need lockstep L2/L3/L4
21+
//! behavior in the meantime can access individual fields and call
22+
//! `.blocks_l2()` / `.map_l2()` / `.bulk_apply_l2()` directly on each
23+
//! `BlockedGrid<FieldT, 64, 64>` field — but be aware that per-field calls
24+
//! are NOT lockstep across fields. If you write per-field loops outside the
25+
//! macro-generated struct, those call sites will not auto-migrate when PR-X3.1
26+
//! lands. Prefer to gate L2-using code on PR-X3.1 if the lockstep guarantee
27+
//! matters.
2028
//!
2129
//! # Reserved field names
2230
//!

src/hpc/blocked_grid/iter.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ pub struct BaseBlockIter<'a, T, const BR: usize, const BC: usize> {
5252
n_block_cols: usize,
5353
}
5454

55-
impl<'a, T: Copy, const BR: usize, const BC: usize> Iterator for BaseBlockIter<'a, T, BR, BC> {
55+
impl<'a, T, const BR: usize, const BC: usize> Iterator for BaseBlockIter<'a, T, BR, BC> {
5656
type Item = GridBlock<'a, T, BR, BC>;
5757

5858
fn next(&mut self) -> Option<Self::Item> {
@@ -84,7 +84,7 @@ impl<'a, T: Copy, const BR: usize, const BC: usize> Iterator for BaseBlockIter<'
8484
}
8585
}
8686

87-
impl<'a, T: Copy, const BR: usize, const BC: usize> ExactSizeIterator for BaseBlockIter<'a, T, BR, BC> {}
87+
impl<'a, T, const BR: usize, const BC: usize> ExactSizeIterator for BaseBlockIter<'a, T, BR, BC> {}
8888

8989
// ============================================================
9090
// BaseBlockIterMut — mutable row-major iterator
@@ -141,7 +141,7 @@ pub struct BaseBlockIterMut<'a, T, const BR: usize, const BC: usize> {
141141
unsafe impl<'a, T: Send, const BR: usize, const BC: usize> Send for BaseBlockIterMut<'a, T, BR, BC> {}
142142
unsafe impl<'a, T: Sync, const BR: usize, const BC: usize> Sync for BaseBlockIterMut<'a, T, BR, BC> {}
143143

144-
impl<'a, T: Copy, const BR: usize, const BC: usize> Iterator for BaseBlockIterMut<'a, T, BR, BC> {
144+
impl<'a, T, const BR: usize, const BC: usize> Iterator for BaseBlockIterMut<'a, T, BR, BC> {
145145
type Item = GridBlockMut<'a, T, BR, BC>;
146146

147147
fn next(&mut self) -> Option<Self::Item> {
@@ -184,13 +184,13 @@ impl<'a, T: Copy, const BR: usize, const BC: usize> Iterator for BaseBlockIterMu
184184
}
185185
}
186186

187-
impl<'a, T: Copy, const BR: usize, const BC: usize> ExactSizeIterator for BaseBlockIterMut<'a, T, BR, BC> {}
187+
impl<'a, T, const BR: usize, const BC: usize> ExactSizeIterator for BaseBlockIterMut<'a, T, BR, BC> {}
188188

189189
// ============================================================
190190
// BlockedGrid methods: blocks_base / blocks_base_mut
191191
// ============================================================
192192

193-
impl<T: Copy, const BR: usize, const BC: usize> BlockedGrid<T, BR, BC> {
193+
impl<T, const BR: usize, const BC: usize> BlockedGrid<T, BR, BC> {
194194
/// Iterator over BR×BC base blocks, yielding one [`GridBlock`] per
195195
/// `(block_row, block_col)` pair in row-major order.
196196
///

0 commit comments

Comments
 (0)