Skip to content

Commit 9d492db

Browse files
committed
fix(pr-x2): split soa_struct! into unpadded + padded arms
P1 codex review on PR #169: unconditional `_logical_len` injection turned previously all-public generated structs into structs with a private field, breaking downstream code that constructs them via struct literals (`MyBatch { a: vec![], b: vec![] }`) and exhaustive pattern matches — even for callers who never use `#[soa(pad_to_lanes = N)]`. Fix: split `soa_struct!` into two arms. Arm 1 — unpadded (no `#[soa(...)]` anywhere): byte-for-byte the pre-PR-X2 emit. No `_logical_len` field. `len()` reads field lengths under `debug_assert`. Struct-literal construction works exactly as before. macro_rules! tries this arm first; if any field carries a `#[soa(pad_to_lanes = N)]` attribute, the no-attribute pattern fails to match and the macro falls through to arm 2. Arm 2 — padded (at least one `#[soa(...)]`): the new PR-X2 B emit with `_logical_len` + lane-padded `push`. Reached only when the user opted in by tagging a field — struct-literal construction was never going to work for these anyway because the user can't fill the lane-padded `Vec` slots themselves. The pre-existing `macro_public_visibility_passthrough` test is restored to its original form (direct `s.x.push(...)` on the unpadded `Soa3`) since arm 1 is byte-identical to pre-PR-X2. Verified: cargo test -p ndarray --lib hpc::soa 38 passed cargo test --doc -p ndarray hpc::soa 14 passed cargo fmt --check clean cargo clippy --features approx,serde,rayon -- -D warnings clean Resolves PR #169 P1 review thread r3272307622.
1 parent 5ee172a commit 9d492db

1 file changed

Lines changed: 80 additions & 9 deletions

File tree

src/hpc/soa.rs

Lines changed: 80 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,74 @@ impl<'a, T, const N: usize> Iterator for SoaChunks<'a, T, N> {
356356
/// ```
357357
#[macro_export]
358358
macro_rules! soa_struct {
359+
// ───────────────────────────────────────────────────────────────────
360+
// Arm 1 — unpadded (no `#[soa(...)]` attribute on any field).
361+
// This is byte-for-byte the pre-PR-X2 emit: no `_logical_len` field,
362+
// `len()` reads from field lengths under `debug_assert`. Existing
363+
// callers (struct-literal construction, exhaustive patterns) are
364+
// unaffected. macro_rules! tries this arm first; if any field has
365+
// a `#[soa(pad_to_lanes = N)]` attribute the pattern fails to match
366+
// and arm 2 is tried.
367+
// ───────────────────────────────────────────────────────────────────
368+
(
369+
$(#[$meta:meta])*
370+
$vis:vis struct $name:ident {
371+
$($field_vis:vis $field:ident : $ty:ty),* $(,)?
372+
}
373+
) => {
374+
$(#[$meta])*
375+
$vis struct $name {
376+
$($field_vis $field: ::std::vec::Vec<$ty>),*
377+
}
378+
379+
impl $name {
380+
/// Construct an empty instance.
381+
pub fn new() -> Self {
382+
Self { $($field: ::std::vec::Vec::new()),* }
383+
}
384+
385+
/// Construct with each field pre-allocated to `cap`.
386+
pub fn with_capacity(cap: usize) -> Self {
387+
Self { $($field: ::std::vec::Vec::with_capacity(cap)),* }
388+
}
389+
390+
/// Append one row across all fields.
391+
#[allow(clippy::too_many_arguments)]
392+
pub fn push(&mut self, $($field: $ty),*) {
393+
$(self.$field.push($field);)*
394+
}
395+
396+
/// Length (all fields share this length; debug-asserted).
397+
pub fn len(&self) -> usize {
398+
let lens = [$(self.$field.len()),*];
399+
debug_assert!(
400+
lens.iter().all(|&l| l == lens[0]),
401+
concat!(stringify!($name), ": field-length invariant violated")
402+
);
403+
lens[0]
404+
}
405+
406+
/// Returns `true` if there are zero rows.
407+
pub fn is_empty(&self) -> bool { self.len() == 0 }
408+
409+
/// Clear all fields. Capacity is retained.
410+
pub fn clear(&mut self) {
411+
$(self.$field.clear();)*
412+
}
413+
}
414+
415+
impl ::std::default::Default for $name {
416+
fn default() -> Self { Self::new() }
417+
}
418+
};
419+
420+
// ───────────────────────────────────────────────────────────────────
421+
// Arm 2 — padded (at least one field has `#[soa(pad_to_lanes = N)]`).
422+
// Adds a `#[doc(hidden)] _logical_len: usize` field so `len()` can
423+
// return the semantic row count independent of lane-tail padding.
424+
// Reached only when arm 1's no-attribute pattern fails to match —
425+
// existing callers without padding never see this struct shape.
426+
// ───────────────────────────────────────────────────────────────────
359427
(
360428
$(#[$meta:meta])*
361429
$vis:vis struct $name:ident {
@@ -371,6 +439,10 @@ macro_rules! soa_struct {
371439
/// Shared logical row count across all fields. Padded fields may
372440
/// have `self.<field>.len() > _logical_len` after `push`.
373441
/// Updated by `push` / `clear`; treat as private.
442+
///
443+
/// Only present on padded structs (at least one field has
444+
/// `#[soa(pad_to_lanes = N)]`); unpadded structs keep the
445+
/// pre-PR-X2 all-public shape.
374446
#[doc(hidden)]
375447
_logical_len: usize,
376448
}
@@ -458,7 +530,8 @@ macro_rules! soa_struct {
458530
$self.$vec[$logical] = $val;
459531
}};
460532

461-
// Internal — plain (unpadded) field push.
533+
// Internal — plain (unpadded) field push inside a padded struct
534+
// (mixed cadence: some fields padded, others not).
462535
(@push_field $self:ident, $vec:ident, $val:ident, $ty:ty, $logical:ident) => {{
463536
$self.$vec.push($val);
464537
}};
@@ -877,16 +950,14 @@ mod tests {
877950
#[test]
878951
fn macro_public_visibility_passthrough() {
879952
// Soa3 has `pub` fields; verify the field is accessible
880-
// (compilation alone proves visibility). Use the macro-generated
881-
// `push` (canonical entry point) so `_logical_len` stays in sync;
882-
// direct `s.x.push(...)` would bypass `_logical_len` since PR-X2 B.
953+
// (compilation alone proves visibility). Soa3 is unpadded → uses
954+
// arm 1 of the macro → fields drive `len()` directly, so pushing
955+
// into individual fields still gives the right count.
883956
let mut s = Soa3::new();
884-
s.push(1.0, 2.0, 3.0);
957+
s.x.push(1.0);
958+
s.y.push(2.0);
959+
s.z.push(3.0);
885960
assert_eq!(s.len(), 1);
886-
// Field access works (visibility test):
887-
assert_eq!(s.x.as_slice(), &[1.0]);
888-
assert_eq!(s.y.as_slice(), &[2.0]);
889-
assert_eq!(s.z.as_slice(), &[3.0]);
890961
}
891962

892963
#[test]

0 commit comments

Comments
 (0)