harm: harm-types crate

[monoid]

Move some definitions to a new harm-types crate.

Define IntoReg trait in harm to avoid foreign crate definition problem.

Closes#12.

Summary by CodeRabbit

• New Features

  • Added a new no-std crate providing shared AArch64 register types and APIs.

• Refactor

  • Moved register implementations into the new crate and updated instruction encoding to use the consolidated register API.

• Chores

  • Added the new crate to the workspace and wired it as a workspace dependency.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a new workspace crate harm-types providing AArch64 register types and a Register trait with index() returning a 5-bit value. The harm crate depends on and re-exports these types; instruction encoders across the codebase were updated to use index() and related IntoReg conversions instead of code().

Changes

Cohort / File(s)	Summary
Workspace & crate addition Cargo.toml, harm-types/Cargo.toml	Adds harm-types as a workspace member and workspace dependency; new crate manifest for harm-types (v0.1.0, metadata, dependencies aarchmrs-types and num_enum).
harm-types public API surface harm-types/src/lib.rs, harm-types/src/A64.rs, harm-types/src/A64/register.rs	New no_std crate exposing A64 module. Implements Register trait (index()), enums Reg64/Reg32/RegOrSp*/RegOrZero*, narrow/extend helpers, TryFrom conversions, RegisterError, BitValue<5> indexing, and unit tests.
harm integration / re-export harm/Cargo.toml, harm/src/register.rs	Adds harm-types dependency and replaces local register definitions with pub use harm_types::A64::register::*;, removing local register types/tests while keeping Sealed impls as applicable.
Arithmetic encoders harm/src/instructions/arith/macros.rs	Encoder macros updated to use .index() for rm, rn, rd extraction instead of .code().
Control-flow encoders harm/src/instructions/control/branch_imm.rs, harm/src/instructions/control/branch_reg.rs, harm/src/instructions/control/testbranch.rs	Replaced .code() with .index() for register operands in CBZ/CBNZ, branch-register, TBZ/TBNZ encoders.
DP immediate encoders harm/src/instructions/dpimm/log_imm.rs, harm/src/instructions/dpimm/movewide.rs	Replaced .code() with .index() for rn/rd operand encoding in logical-immediate and move-wide instructions.
DP register (log-shift) encoders harm/src/instructions/dpreg/log_shift.rs	Switched trait bounds from Into<$rn> to IntoReg<$rn>, updated constructors to use into_reg(), added IntoReg import, and replaced .code() with .index() for mask, rn, rd.
Load/store encoders harm/src/instructions/ldst/macros.rs	Replaced .code() with .index() for offset, base, and rt register access across reg/imm/unscaled/PC/pair offset variants generated by macros.

Sequence Diagram(s)

sequenceDiagram
participant Harm as harm crate
participant Types as harm_types::A64::register
participant Encoder as instruction encoder
Note over Harm,Types: New dependency & re-export
Harm->>Types: reference Reg/RegOr* types
Harm->>Encoder: provide instruction with register operand
Encoder->>Types: call .index()
Types-->>Encoder: BitValue<5>
Encoder-->>Harm: assembled instruction code

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas to inspect closely:

• harm-types/src/A64/register.rs: correctness of index encoding, NICHE_REG (31) handling, TryFrom edge cases, and unit tests.
• harm/src/register.rs: re-export correctness and Sealed impl compatibility across crate.
• dpreg/log_shift.rs: IntoReg trait changes and all call sites using into_reg().
• Scan all encoder files to ensure all .code() → .index() migrations are consistent and no regressions in encoding construction.

Poem

🐇 I hopped a crate across the land,
Five-bit hops now neat and planned,
Types moved out with gentle cheer,
Encoders call index, tidy and clear,
A tiny refactor — carrot-coded cheer!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.21% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Out of Scope Changes check	❓ Inconclusive	The PR includes changes to replace code() calls with index() calls across multiple files, but the scope relationship to issue #12 (creating harm-types crate) is unclear.	Clarify whether the replacement of code() with index() calls is part of integrating the new harm-types crate or represents a separate refactoring effort beyond the scope of issue #12.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: introducing a new harm-types crate by moving register type definitions to it.
Linked Issues check	✅ Passed	The PR successfully fulfills issue #12 by creating the harm-types crate and splitting register type definitions into it for reuse.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch harm/harm-types

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

harm/src/register.rs (1)

7-7: Consider narrowing the public re-export instead of using a glob

pub use harm_types::A64::register::*; cleanly preserves the old surface, but it also makes harm’s public API sensitive to any future additions in harm-types (e.g., helper types/constants you may not want to expose). If API stability is a concern, consider listing the exported items explicitly here instead of using *.

harm-types/src/A64/register.rs (2)

8-22: Minor doc polish for clarity and typos

The high-level docs are very helpful. There are a couple of small textual issues you might want to fix when convenient:

• “general-propose” → “general-purpose” (appears multiple times).
• The phrase “or XZR/WZR as SP/WSP” is a bit hard to parse; consider rephrasing to more clearly distinguish the SP vs zero-register interpretations.
  These are non-functional, purely documentation nits.

---

167-209: Prefer or_else over or in TryFrom<u8> for lazy error handling

The TryFrom<u8> impls for RegOrSp* / RegOrZero* currently use Reg{32,64}::try_from(value).map(Into::into).or({ ... }). Because Result::or takes a Result, the { ... } branch is evaluated even when try_from succeeds, which is a minor inefficiency on the happy path.

You can switch to or_else so the fallback is only computed on error, e.g.:

- Reg64::try_from(value).map(Into::into).or({- if value == NICHE_REG {- Ok(Self::SP)- } else {- Err(RegisterError::InvalidRegisterCode(value))- }- })+ Reg64::try_from(value).map(Into::into).or_else(|_| {+ if value == NICHE_REG {+ Ok(Self::SP)+ } else {+ Err(RegisterError::InvalidRegisterCode(value))+ }+ })

(Similarly for the other three implementations.)

Also applies to: 342-370

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8dfafb8 and 3434fc7.

📒 Files selected for processing (15)

• Cargo.toml (2 hunks)
• harm-types/Cargo.toml (1 hunks)
• harm-types/src/A64.rs (1 hunks)
• harm-types/src/A64/register.rs (1 hunks)
• harm-types/src/lib.rs (1 hunks)
• harm/Cargo.toml (1 hunks)
• harm/src/instructions/arith/macros.rs (3 hunks)
• harm/src/instructions/control/branch_imm.rs (2 hunks)
• harm/src/instructions/control/branch_reg.rs (3 hunks)
• harm/src/instructions/control/testbranch.rs (2 hunks)
• harm/src/instructions/dpimm/log_imm.rs (8 hunks)
• harm/src/instructions/dpimm/movewide.rs (3 hunks)
• harm/src/instructions/dpreg/log_shift.rs (10 hunks)
• harm/src/instructions/ldst/macros.rs (11 hunks)
• harm/src/register.rs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

harm/src/instructions/ldst/macros.rs (1)

harm-types/src/A64/register.rs (7)

• index (32-32)
• index (374-376)
• index (381-383)
• index (388-393)
• index (398-403)
• index (408-413)
• index (418-423)

harm/src/instructions/control/branch_reg.rs (1)

aarchmrs-instructions/src/A64/control/branch_reg.rs (3)

• RET_64R_branch_reg (124-130)
• BR_64_branch_reg (16-22)
• BLR_64_branch_reg (70-76)

harm/src/instructions/dpimm/movewide.rs (2)

harm/src/instructions/control/branch_imm.rs (5)

• to_code (102-105)
• to_code (138-141)
• to_code (183-186)
• to_code (267-275)
• to_code (280-288)

harm/src/instructions/control/branch_reg.rs (3)

• to_code (39-41)
• to_code (48-50)
• to_code (62-64)

harm/src/instructions/dpreg/log_shift.rs (2)

harm/src/instructions/dpimm/log_imm/args.rs (8)

• new (22-28)
• new (39-45)
• new (56-62)
• new (73-79)
• new (90-96)
• new (107-113)
• new (124-130)
• new (141-147)

harm/src/register.rs (9)

• into_reg (17-17)
• into_reg (22-24)
• into_reg (29-31)
• into_reg (36-38)
• into_reg (43-45)
• into_reg (50-52)
• into_reg (57-59)
• into_reg (64-66)
• into_reg (71-73)

harm/src/instructions/arith/macros.rs (1)

harm-types/src/A64/register.rs (7)

• index (32-32)
• index (374-376)
• index (381-383)
• index (388-393)
• index (398-403)
• index (408-413)
• index (418-423)

🔇 Additional comments (26)

harm/Cargo.toml (2)

16-16: LGTM: Workspace dependency correctly added.

The harm-types workspace dependency integration is properly configured.

---

4-4: No action needed—Rust edition 2024 is stable.

Rust Edition 2024 shipped with Rust 1.85 (stable release on February 20, 2025), so the edition specification is valid and correct.

Cargo.toml (2)

9-9: LGTM: Workspace configuration correctly updated.

The harm-types crate is properly integrated as a workspace member with matching version and path configuration.

Also applies to: 25-25

---

17-17: Resolver "3" is valid and fully supported.

Resolver "3" is the latest Cargo resolver version, stabilized as an opt-in resolver in Rust 1.84.0 (Jan 9, 2025). It became the default for the Rust 2024 edition in Rust 1.85.0 (Feb 20, 2025). This configuration is correct and requires no changes.

harm-types/Cargo.toml (1)

1-15: Crate manifest structure looks good.

The package metadata and dependencies are appropriately configured for a types-only crate.

harm/src/instructions/control/testbranch.rs (2)

57-75: LGTM: Register encoding updated to use index().

The change from code() to index() for 64-bit test-branch encoding is consistent with the new register API.

---

77-90: LGTM: Register encoding updated to use index().

The change from code() to index() for 32-bit test-branch encoding is consistent with the new register API.

harm-types/src/A64.rs (1)

1-6: LGTM: Clean module structure.

The A64 module properly exposes the register submodule as part of the public API.

harm/src/instructions/control/branch_imm.rs (2)

265-276: LGTM: Register encoding updated to use index().

The change from code() to index() for 64-bit compare-branch encoding aligns with the new register API.

---

278-289: LGTM: Register encoding updated to use index().

The change from code() to index() for 32-bit compare-branch encoding aligns with the new register API.

harm/src/instructions/control/branch_reg.rs (3)

37-42: LGTM: Register encoding updated to use index().

The RET instruction encoding now uses index() consistently with the new register API.

---

46-51: LGTM: Register encoding updated to use index().

The BR instruction encoding now uses index() consistently with the new register API.

---

60-65: LGTM: Register encoding updated to use index().

The BLR instruction encoding now uses index() consistently with the new register API.

harm/src/instructions/dpimm/movewide.rs (3)

164-168: LGTM: Register encoding updated to use index().

The MOVK instruction encoding now uses index() for both 32-bit and 64-bit variants, consistent with the new register API.

Also applies to: 170-174

---

190-194: LGTM: Register encoding updated to use index().

The MOVN instruction encoding now uses index() for both 32-bit and 64-bit variants, consistent with the new register API.

Also applies to: 196-200

---

216-220: LGTM: Register encoding updated to use index().

The MOVZ instruction encoding now uses index() for both 32-bit and 64-bit variants, consistent with the new register API.

Also applies to: 222-226

harm/src/instructions/dpimm/log_imm.rs (1)

30-31: index() migration for DPIMM logical-immediate encodings looks consistent

All rn/rd operands now use .index() on RegOrSp* / RegOrZero*, with Register in scope, and argument ordering into the *_log_imm helpers is unchanged. Given the comprehensive LOG_IMM tests below, this should preserve exact encodings while aligning with the new Register::index API.

Also applies to: 42-43, 54-55, 67-68, 79-80, 92-93, 104-105, 117-118

harm-types/src/lib.rs (1)

1-19: Crate attributes and module surface are well-chosen for harm-types

no_std, forbid(unsafe_code), and the deny/allow set are appropriate for a small register-types crate, and exporting A64 as the root module gives a clear, minimal public surface.

harm/src/instructions/ldst/macros.rs (1)

69-82: Load/store macros correctly switched to index()-based register encoding

Across reg-offset, (scaled/unscaled) imm-offset, PC-relative, and pair offset variants, all base/offset/rt operands now use .index() on RegOrSp64 / RegOrZero* while preserving the previous call signatures and argument ordering to the aarchmrs_instructions helpers. This keeps encodings stable and centralizes the 5‑bit mapping in the Register impls.

Also applies to: 83-95, 97-109, 233-260, 331-337, 383-388, 559-599

harm/src/instructions/arith/macros.rs (1)

156-172: Arithmetic instruction encoders now consistently use Register::index

For shifted, immediate, and extend forms, rm, rn, and rd are taken via .index() on the register types, with the same bitfield wiring into the underlying encoder functions. This matches the new register API and should be a no-op for the actual encodings.

Also applies to: 220-233, 312-323

harm/src/instructions/dpreg/log_shift.rs (2)

23-24: IntoReg-based logical-args builders are sound and avoid orphan issues

Switching Make*LogicalArgs to require IntoReg<$rn> and using into_reg() for rd, rn, and the mask register keeps the public API ergonomic (e.g., still accepts plain Reg32/Reg64) while routing conversions through your local IntoReg trait instead of foreign Into. That’s the right pattern for sharing register types from harm-types without hitting the orphan rules.

Also applies to: 31-33, 39-41, 49-51, 61-63

---

95-109: log-shift encoders correctly migrated to index()-based operands

All DPREG logical-shift variants (and, ands, eor, orr, 32/64‑bit) now feed mask, rn, and rd via .index() on RegOrZero*, with the shift kind and amount handling unchanged. Given the extensive LOG_SHIFT_DB tests below, this looks like a safe mechanical migration to the new Register trait.

Also applies to: 118-127, 136-145, 158-163, 176-181, 194-199, 211-217, 230-235

harm-types/src/A64/register.rs (4)

35-48: Check MSRV impact of using core::error::Error

Implementing ::core::error::Error on RegisterError is nice for no_std, but it ties the crate to the Rust version where core::error::Error was stabilized. Please confirm that this aligns with your intended MSRV; if you need an older MSRV, you may have to fall back to std::error::Error behind a std feature or skip the Error impl.

---

89-127: Explicit narrow/extend mappings look correct and type-safe

The Reg64::narrow and Reg32::extend match arms cover all defined general-purpose registers and keep LR/WLR consistent across sizes. The explicit matches avoid relying on numeric assumptions and should be easy to audit/maintain.

Also applies to: 250-287

---

372-424: Register::index implementations match the encoding model

All Register impls return a 5-bit BitValue derived from the underlying repr(u8) discriminant or NICHE_REG for SP/XZR/WSP/WZR, which aligns with the architectural encoding (0–30 for GP regs and 31 for the special case). This looks correct and consistent across 32- and 64-bit variants.

---

426-566: Solid test coverage for conversions and size transitions

The tests exercise:

• TryFrom<u8> success/failure/NICHE_REG paths for all register flavours.
• narrow/extend for both plain and SP/zero variants, including LR/WLR.

This gives good confidence in the mapping logic and boundary handling around code 31.

[monoid]

@CodeRabbit clippy complains about using or_else here because or expression is very simple (no side effects, no allocation).

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for or_else usage in the repository
rg -n 'or_else' --type rust

Length of output: 932

---

🏁 Script executed:

#!/bin/bash
# Search for or_else specifically in harm-types directory
rg -n 'or_else' harm-types/
# Also check if mov_imm.rs was modified in this PR by looking at git diff
git diff master...HEAD --name-only | grep -i mov

Length of output: 35

---

🏁 Script executed:

#!/bin/bash
# Check harm/src/instructions/mov/mov_imm.rs for context around or_else usage
cat harm/src/instructions/mov/mov_imm.rs | head -n 250

Length of output: 7544

---

@monoid, I can see the issue. At lines 160 and 238 in harm/src/instructions/mov/mov_imm.rs, you're using or_else(movk) where movk is a closure that performs simple computations without side effects or allocations. Clippy's unnecessary_lazy_evaluation lint suggests using eager evaluation instead.

Change this:

movz.or_else(movk).ok_or(InvalidMovImm)

To this:

movz.or(movk()).ok_or(InvalidMovImm)

This applies to both occurrences:

• Line 160 in try_into_mov_z_or_k_32 (32-bit version)
• Line 238 in try_into_mov_z_or_k_64 (64-bit version)

Since movk() just performs bit operations and pattern matching without allocations, it's safe and more idiomatic to evaluate it eagerly with .or() rather than lazily with .or_else().

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

harm-types/src/A64/register.rs (4)

6-22: Minor doc wording/clarity nit in module-level comments

The docs are very clear overall; just a small phrasing tweak could improve readability:

• Line 18 reads: “where XZR/WZR is allowed, use RegOrZero64 or RegOrZero32 is used.” — the trailing “is used” is redundant. Consider rephrasing to something like:

//! where `XZR`/`WZR` is allowed, use `RegOrZero64` or `RegOrZero32`.

Purely cosmetic; no behavior impact.

---

181-208: Use or_else for lazy fallback in TryFrom<u8> implementations

The TryFrom<u8> impls for RegOrSp64, RegOrZero64, RegOrSp32, and RegOrZero32 are logically correct, but Result::or eagerly evaluates the fallback branch even when the initial Reg*::try_from succeeds. Using or_else would keep the same behavior while avoiding unnecessary work and making the “fallback on error” intent a bit clearer.

For example, for RegOrSp64:

- Reg64::try_from(value).map(Into::into).or({- if value == NICHE_REG {- Ok(Self::SP)- } else {- Err(RegisterError::InvalidRegisterCode(value))- }- })+ Reg64::try_from(value).map(Into::into).or_else(|_| {+ if value == NICHE_REG {+ Ok(Self::SP)+ } else {+ Err(RegisterError::InvalidRegisterCode(value))+ }+ })

And analogously for RegOrZero64, RegOrSp32, and RegOrZero32.

This keeps the same observable semantics (including error values) but avoids constructing the fallback Result on the happy path.

Also applies to: 342-370

---

372-424: Register::index implementations align with encoding constraints

All Register impls map variants to BitValue<5> using the underlying u8 discriminant or the NICHE_REG constant for SP/XZR/WSP/WZR. Given the enums are #[repr(u8)] with values 0–30 and NICHE_REG = 31, this stays within 5 bits, which fits the stated “5-bit number” contract.

You might optionally add a brief comment near NICHE_REG noting that it is expected to be within the BitValue<5> range (0–31), but behavior-wise this looks solid.

---

426-566: Test coverage is strong; optional additions for index()

The tests thoroughly exercise TryFrom<u8> paths and the narrow/extend conversions, including the NICHE_REG boundary cases. That gives good confidence in the register modeling.

If you want slightly more safety against future refactors, you could add a couple of small tests around Register::index() (e.g., X0→0, LR→30, SP/XZR/WSP/WZR→31) to pin the encoding contract explicitly. Not required given how direct the implementations are, just an optional safeguard.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3434fc7 and 7e529a1.

📒 Files selected for processing (2)

• harm-types/Cargo.toml (1 hunks)
• harm-types/src/A64/register.rs (1 hunks)

🔇 Additional comments (2)

harm-types/Cargo.toml (1)

1-15: Crate manifest looks consistent and focused

Package metadata, workspace wiring, and dependency declarations all look sane for a non-published internal types crate. I don’t see issues here.

harm-types/src/A64/register.rs (1)

50-288: Register enums and narrow/extend mappings look correct

The Reg64/Reg32 definitions and the narrow/extend implementations appear exhaustive and symmetric (X0↔W0 … LR↔WLR), and the separation from SP/XZR/WSP/WZR via the RegOr* types matches AArch64’s encoding model.

No functional issues spotted here.

[monoid]

At lines 160 and 238 in harm/src/instructions/mov/mov_imm.rs, you're using or_else(movk) where movk is a closure that performs simple computations without side effects or allocations

No, clippy doesn't complain here, probably because the expression is much more complex than simple value construction. It complains about your proposition to use or_else in the TryFrom<u8> implementations.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

harm-types/src/A64/register.rs (2)

181-209: Consider simplifying the TryFrom<u8> patterns to avoid double error construction

The TryFrom<u8> impls for RegOrSp*/RegOrZero* are correct, but the RegX::try_from(value).map(Into::into).or({ ... }) style creates an error twice for invalid codes (RegX::try_from and the final Err(RegisterError::InvalidRegisterCode(value))).

A slightly more direct pattern would be:

implTryFrom<u8>forRegOrSp64{typeError = RegisterError;fntry_from(value:u8) -> Result<Self,Self::Error>{match value {
v @ 0..=30 => Reg64::try_from(v).map(RegOrSp64::Reg),
v if v == NICHE_REG => Ok(RegOrSp64::SP),
v => Err(RegisterError::InvalidRegisterCode(v)),}}}

and analogously for the other three types. This keeps the intent explicit and avoids constructing an intermediate error that’s thrown away, while preserving the same behavior.

Also applies to: 342-370

---

426-587: Tests give very good coverage; only tiny naming nit

The tests exercise:

• TryFrom<u8> happy‑path and error cases for all register families.
• All narrow()/extend() combinations, including SP/XZR/WSP/WZR cases.
• index() values for both 32‑ and 64‑bit variants and their “or SP/zero” wrappers.

That’s strong coverage for what’s otherwise subtle encoding logic. The only tiny nit: names like test_reg64_or_sp64_from_u8_invalid actually test RegOrSp64; if you ever touch these again, aligning names (e.g., test_reg_or_sp64_from_u8_invalid) would make them slightly clearer to future readers. Functionally, everything looks solid.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e529a1 and c242973.

📒 Files selected for processing (1)

• harm-types/src/A64/register.rs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

harm-types/src/A64/register.rs (1)

aarchmrs-types/src/bit_value.rs (1)

• into_inner (37-39)

🔇 Additional comments (3)

harm-types/src/A64/register.rs (3)

30-48: Register trait and error type look well‑scoped for this crate

Register returning a BitValue<5> cleanly expresses the architectural encoding, and RegisterError::InvalidRegisterCode(u8) is a focused error type that composes well with the TryFrom impls. No issues here from a correctness or design standpoint.

---

50-127: Explicit 32/64‑bit mappings are correct and easy to audit

The Reg64/Reg32 enums plus narrow()/extend() matches look exhaustive and symmetric, including the LR/WLR mapping. Given how error‑prone encoding bugs can be, the explicit matches here are a good trade‑off vs. relying on casts/transmutes.

Also applies to: 211-288

---

372-424: Register::index() implementations align with AArch64 encodings

All index() impls consistently map architectural register codes to 0–31, with LR/WLR → 30 and SP/XZR/WSP/WZR → 31 via NICHE_REG. This matches the encoding described in the module docs and should integrate cleanly with the encoder paths using BitValue<5>.