Skip to content

chore(ggml-spanker): polish — checked_mul in test helper, doc bindgen layout_tests, OUTPUT_ELEM_BYTES re-export #16

@marcos-mendez

Description

@marcos-mendez

Context

Bundles the 5 deferred MEDIUM/LOW findings from the Agent R review of Spanker PR #15 (which merged the bindgen integration + 5 review nits + 10 new tests). None merge-blocking but worth a focused cleanup.

Tasks

MEDIUM 1 — Use checked_mul in test allocator helper

src/backends/ggml/tests/mock_matmul.rs alloc_operands uses bare * arithmetic on usize. The production helpers (expected_a_bytes, expected_b_bytes, expected_out_bytes) correctly use checked_mul. The test helper signals the wrong idiom for a future contributor copying it.

Fix: replace bare * with checked_mul().unwrap(), OR call the production helpers directly.

MEDIUM 2 — Re-export OUTPUT_ELEM_BYTES for test usage

OUTPUT_ELEM_BYTES is declared as a local const in the integration test file (4 = sizeof::()). Today it duplicates the crate-level value with no enforcement that they match.

Fix: re-export OUTPUT_ELEM_BYTES as pub (the crate-level is already pub(crate)), import it in the integration test. Or use std::mem::size_of::<f32>() inline + comment.

MEDIUM 3 — matmul_q4_k_returns_not_implemented silent skip

The test silently returns when /dev/null open fails, emitting only eprintln!. A skipped test that produces no visible failure is indistinguishable from a passing test in CI summaries.

Fix: refactor so the stub test doesn't need a file-system handle (the stub ignores ctl entirely). Pass a mock SpankerControl if API allows, or document the skip path explicitly with cargo test ignored marker.

LOW 4 — Document bindgen.layout_tests(false)

build.rs calls .layout_tests(false) which suppresses bindgen's compile-time size/align assertions. Safe today (only struct spanker_version is bound and its layout is manually asserted in the cross-mirror test) but the rationale is undocumented.

Fix: one-line comment in build.rs explaining: layout assertions would require the test runner to link against libclang; the mirror test in lib.rs covers size_of::<ffi::spanker_version>() instead.

LOW 5 — Pin bindgen patch version

Cargo.toml declares bindgen = "0.69" (allows 0.69.x). Cargo.lock pins 0.69.5. Bindgen patch releases have historically changed generated-code layout.

Fix: pin to =0.69.5 OR document the current approach (# Cargo.lock is the source of truth for bindgen patch version).

Acceptance

  • All 5 changes in one PR
  • Tests still 42/42 pass
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo fmt --check --all clean

Refs

Authored by Agent R (Reviewer).

Metadata

Metadata

Assignees

No one assigned

    Labels

    stream-3Software Stack (Agent 3) — driver, runtime, GGML, Spanker

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions