Skip to content

Commit c317041

Browse files
committed
fix(pr-x1): two P2 codex findings on PR #167
1. array_chunks_checked: guard N == 0 before modulo `data.len() % 0` would panic via `slice::as_chunks::<0>()` (and the modulo itself). The strict-fallible contract folds N==0 into Err so callers on the checked surface never see an unexpected panic. New test `array_chunks_checked_rejects_zero_n` covers the edge. 2. Fingerprint<8>::as_u8x64: gate to target_endian = "little" The pointer-reinterpret returns a native-endian view; on a BE target the byte order would contradict the project-wide LE convention used by Fingerprint::to_bytes / from_bytes (both `u64::to_le_bytes` / `from_le_bytes`). `.cargo/config.toml` pins `target-cpu=x86-64-v4` so all supported targets are LE in practice — the cfg gate just makes the LE assumption explicit instead of implicit. SAFETY comment item 6 now cites the gate. The accompanying `pr_x1_as_u8x64_tests` module is gated to LE to match. Both fixes per codex review threads on PR #167.
1 parent 6b52a46 commit c317041

2 files changed

Lines changed: 41 additions & 0 deletions

File tree

src/hpc/fingerprint.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,19 @@ impl Fingerprint<8> {
650650
/// For other `N` widths, use [`Fingerprint::chunks_u8x64`] which yields
651651
/// 64-byte chunks without the compile-time length guarantee.
652652
///
653+
/// # Endianness contract — little-endian only
654+
///
655+
/// Gated to `#[cfg(target_endian = "little")]` so the returned byte view
656+
/// matches the project-wide little-endian word convention used by
657+
/// [`Fingerprint::to_bytes`] / [`Fingerprint::from_bytes`]
658+
/// (both use `u64::to_le_bytes` / `u64::from_le_bytes`).
659+
/// On a big-endian target the raw memory layout of `[u64; 8]` would put
660+
/// the high byte first, contradicting the LE contract and breaking
661+
/// cross-platform SIMD consumers. The `.cargo/config.toml` pins
662+
/// `target-cpu=x86-64-v4`, so all supported targets (x86_64 + aarch64)
663+
/// are little-endian; the cfg gate makes the LE assumption explicit
664+
/// rather than implicit. See P2 review on PR #167.
665+
///
653666
/// # Design reference
654667
///
655668
/// `.claude/knowledge/pr-x1-design.md` § "2. `Fingerprint::as_u8x64`".
@@ -679,6 +692,7 @@ impl Fingerprint<8> {
679692
/// assert_eq!(view[7], 0x01);
680693
/// assert_eq!(view[8], 0x00); // word[1] is zero
681694
/// ```
695+
#[cfg(target_endian = "little")]
682696
#[inline]
683697
pub fn as_u8x64(&self) -> &[u8; 64] {
684698
// SAFETY:
@@ -696,11 +710,14 @@ impl Fingerprint<8> {
696710
// 5. The returned reference borrows from `&self`, so its lifetime cannot
697711
// outlive `self`, satisfying the borrow-checker lifetime rule and
698712
// preventing dangling references.
713+
// 6. The `#[cfg(target_endian = "little")]` gate guarantees the byte
714+
// order matches the project-wide LE convention (P2 review fix #167).
699715
unsafe { &*(self.words.as_ptr() as *const [u8; 64]) }
700716
}
701717
}
702718

703719
#[cfg(test)]
720+
#[cfg(target_endian = "little")]
704721
mod pr_x1_as_u8x64_tests {
705722
use super::*;
706723

src/simd_ops.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,13 @@ pub fn array_chunks<T, const N: usize>(data: &[T]) -> impl Iterator<Item = &[T;
337337
/// the buffer is lane-aligned and wants the error surfaced rather than
338338
/// silently truncating.
339339
///
340+
/// # Edge case — `N == 0`
341+
///
342+
/// Returns `Err(())` when `N == 0`. The underlying `slice::as_chunks::<0>`
343+
/// would panic on a zero divisor; the strict-fallible contract here folds
344+
/// that into the existing error path so callers never see an unexpected
345+
/// panic on the checked surface.
346+
///
340347
/// # Examples
341348
///
342349
/// ```
@@ -347,11 +354,19 @@ pub fn array_chunks<T, const N: usize>(data: &[T]) -> impl Iterator<Item = &[T;
347354
///
348355
/// let bad: Vec<u8> = (0..7).collect();
349356
/// assert!(array_chunks_checked::<u8, 4>(&bad).is_err());
357+
///
358+
/// // N == 0 surfaces as Err rather than panicking.
359+
/// assert!(array_chunks_checked::<u8, 0>(&[0u8; 8]).is_err());
350360
/// ```
351361
#[inline]
352362
pub fn array_chunks_checked<T, const N: usize>(
353363
data: &[T],
354364
) -> Result<impl Iterator<Item = &[T; N]> + '_, ()> {
365+
if N == 0 {
366+
// `data.len() % 0` would panic; the strict-fallible contract
367+
// folds N==0 into Err. See P2 review on PR #167.
368+
return Err(());
369+
}
355370
if data.len() % N != 0 {
356371
return Err(());
357372
}
@@ -401,4 +416,13 @@ mod array_chunks_tests {
401416
let it = array_chunks_checked::<u8, 4>(&[]).expect("0 % 4 == 0, should be Ok");
402417
assert_eq!(it.count(), 0);
403418
}
419+
420+
#[test]
421+
fn array_chunks_checked_rejects_zero_n() {
422+
// P2 review fix on PR #167: N == 0 must surface as Err, not panic
423+
// via `data.len() % 0`.
424+
assert!(array_chunks_checked::<u8, 0>(&[]).is_err());
425+
assert!(array_chunks_checked::<u8, 0>(&[0u8; 8]).is_err());
426+
assert!(array_chunks_checked::<u32, 0>(&[1u32, 2, 3]).is_err());
427+
}
404428
}

0 commit comments

Comments
 (0)