[bab2min]

Maybe fixes #290

Summary by CodeRabbit

• New Features
  • Apple framework builds now support universal binaries for macOS and iOS Simulator, combining supported architectures into a single framework.
  • ARM64 Apple builds now use the appropriate optimized processing path.
• Bug Fixes
  • Invalid or unsupported model data is now rejected with serialization-specific errors, including data that is too small to contain a valid model header.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5c9745e2-690b-48c6-be79-d64bea18845e

📥 Commits

Reviewing files that changed from the base of the PR and between f1a264a and e62b548.

📒 Files selected for processing: 5

• CMakeLists.txt
• bindings/swift/scripts/build-xcframework.sh
• src/ArchUtils.cpp
• src/CoNgramModel.cpp
• src/CoNgramModel.hpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The changes update Apple architecture selection and XCFramework assembly, and adjust CoNgram model validation and exception types. The CoNgramState forward declaration also changes from class to struct.

Changes: Apple architecture builds

Layer / File(s)	Summary
Apple architecture selection and NEON dispatch CMakeLists.txt, src/ArchUtils.cpp	CMake selects arm64 for Apple arm64 or arm64e targets and x86_64 for Apple x86_64 targets. The ARM64 compile definition moves to the target, and the NEON selection branch recognizes KIWI_ARCH_ARM64.
Architecture-specific framework assembly bindings/swift/scripts/build-xcframework.sh	The script checks for the Release static library after each build. It creates iOS Simulator and macOS frameworks from separate arm64 and x86_64 builds, combining their libraries with lipo. iOS Device remains an arm64 build.

CoNgram model loading

Layer / File(s)	Summary
Model validation and exception types src/CoNgramModel.hpp, src/CoNgramModel.cpp	Model creation rejects buffers smaller than the model header before reading it. Unsupported window sizes, key sizes, and architectures now raise SerializationException. The CoNgramState forward declaration changes from class to struct.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant BuildScript as build-xcframework.sh
  participant CMake
  participant Lipo as lipo
  BuildScript->>CMake: Build an architecture-specific target
  CMake-->>BuildScript: Return the Release static library
  BuildScript->>Lipo: Combine libraries for universal frameworks

Loading

Merge Risk: ⚪ Minimal · up to e62b5

No actionable issue remains identified in the Apple build or model-loading changes. The change is mergeable after normal build checks.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The changes in src/CoNgramModel.cpp replace exception types for unsupported model inputs, and the CoNgramState forward declaration changes from class to struct. The supplied issue [#290] conce…	Remove the unrelated src/CoNgramModel.cpp exception changes and the CoNgramState declaration change, or provide a directly linked coding requirement that requires them.
Docstring Coverage		Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title accurately identifies the primary change: fixing Apple platform builds, with a focus on macOS ARM64 architecture handling. It is concise, although it does not mention the related XCFramework…
Linked Issues check		Issue [#290] requires the ARM64 XCFramework path to select the existing NEON/int8 implementation. CMakeLists.txt maps single-architecture Apple arm64 and arm64e builds to KIWI_CPU_ARCH=arm64, …

Full details: Title check

Explanation

The title accurately identifies the primary change: fixing Apple platform builds, with a focus on macOS ARM64 architecture handling. It is concise, although it does not mention the related XCFramework and iOS simulator changes.

Full details: Linked Issues check

Explanation

Issue [#290] requires the ARM64 XCFramework path to select the existing NEON/int8 implementation. CMakeLists.txt maps single-architecture Apple arm64 and arm64e builds to KIWI_CPU_ARCH=arm64, adds the NEON source, and defines KIWI_ARCH_ARM64=1. src/ArchUtils.cpp uses that definition to select NEON when CPUINFO is disabled. build-xcframework.sh builds ARM64 and x86_64 slices separately and merges each platform with lipo, so the ARM64 slice receives the ARM-specific source and definition. Issue [#290] has no stated automated-test acceptance criterion. The supplied evidence has no build validation result, but the implementation directly addresses the reported dispatch and packaging path.

Full details: Out of Scope Changes check

Explanation

The changes in src/CoNgramModel.cpp replace exception types for unsupported model inputs, and the CoNgramState forward declaration changes from class to struct. The supplied issue [#290] concerns ARM64 architecture detection and XCFramework packaging. These changes have no demonstrated connection to that objective.

Resolution

Remove the unrelated src/CoNgramModel.cpp exception changes and the CoNgramState declaration change, or provide a directly linked coding requirement that requires them.

Full details: Docstring Coverage

Explanation

Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}