Skip to content

Simplify decoders, dedupe public/internal types, require Go 1.27 - #1

Merged
jenska merged 3 commits into
mainfrom
simplify-decoders
Sep 3, 2026
Merged

jenska merged 3 commits into
mainfrom
simplify-decoders

Conversation

@jenska

@jenska jenska commented Sep 3, 2026 •

Copy link
Copy Markdown
Owner

What changed

1. Remove the duplicated type layer (−220 lines)

The structured decode types (Operand, Register, EffectiveAddress, ImmediateValue and their Kind enums) were defined twice — once in public_types.go, once in internal/decoders/types.go — with ~150 lines of convertMetadata / convertOperand / cloneUint32Ptr / cloneInt32Ptr in disasm.go copying between the two structurally identical type sets on every decode. public_types.go now re-exports the decoder types via type aliases (type Operand = decoders.Operand, etc.), so there is one source of truth and finalizeInstruction assigns Metadata directly. The public API surface is unchanged (aliases are identical types; external callers cannot import internal/).

2. Collapse repeated decoder logic

  • CMPI / ANDI / ORI / EORI reuse the existing decodeImmediateBinaryOp; the trivial decodeLogical wrapper is gone
  • CMPA and the ADDA / SUBA path share a new decodeAddressRegisterOp
  • JSR / JMP / PEA share decodeUnaryEA
  • ABCD / SBCD / CMPM share addrIndirectOperand
  • decodeBxx and decodeShiftRotate merge their duplicated tail branches into one setInstruction call
  • the 40-line inline DC.W fallback literal in disasm.go becomes decoders.DecodeUnknown(...)
  • formatRegisterList replaces its mirrored D/A loops with a bitFor closure
  • misc: dead bounds check in getSizeString, maskBitOp alias of maskFFC0, hand-rolled joinOperands → strings.Join, decodeIndexWord tidy

3. Require Go 1.27

  • go.mod go 1.26 → go 1.27; CI matrix 1.25.x → 1.27.x
  • uint32Ptr / int32Ptr helpers replaced with the Go 1.26 new(expr) form and deleted
  • range-over-int in formatRegisterList
  • preallocate the instruction slice in DisassembleRange (>= len(data)/2)
  • delete stray root go.yml (duplicate of .github/workflows/ci.yml, never executed by GitHub Actions since it is not under .github/workflows/)

Why

vereinfache den code + aktualisiere auf go 1.27. The convert/clone layer was pure boilerplate risk with no behavioral purpose, and most instruction decoders had near-identical copies of the same EA/immediate handling.

For reviewers

  • No behavior change. The exhaustive TestFindDecoderMatchesOpcodeTable (all 65536 opcodes) and the full decoder test suite pass unchanged; go vet and gofmt are clean.
  • Non-test code drops from 2266 to ~2000 lines.
  • finalizeInstruction now shares the decoder's Metadata slice instead of deep-copying it. Safe because decoderInst is function-local and discarded after the call; the symbolizer path only reads the operand slice and rewrites the Operands string.
  • DisassembleRange now returns a non-nil empty slice (instead of nil) when it decodes nothing; callers in the tree only check len().

🤖 Generated with Claude Code

The structured decode types (Operand, Register, EffectiveAddress,
ImmediateValue and their Kind enums) were defined twice - once in
public_types.go and once in internal/decoders/types.go - with ~150 lines
of convert*/clone* glue in disasm.go copying between the two identical
type sets on every decode. public_types.go now re-exports the decoder
types via type aliases, so there is a single source of truth and
finalizeInstruction assigns metadata directly.

Also collapses repeated decoder logic:
- CMPI/ANDI/ORI/EORI reuse the existing decodeImmediateBinaryOp
- CMPA and the ADDA/SUBA path share decodeAddressRegisterOp
- JSR/JMP/PEA share decodeUnaryEA
- ABCD/SBCD/CMPM share addrIndirectOperand
- decodeBxx and decodeShiftRotate merge their duplicated tail branches
- the DC.W fallback literal becomes decoders.DecodeUnknown
- misc: getSizeString bounds check, maskBitOp alias, joinOperands

No behavior change: the exhaustive FindDecoder test over all 65536
opcodes and the decoder test suite pass unchanged; go vet and gofmt clean.
Non-test code drops from 2266 to 2014 lines.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9adae8bba1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread disasm.go
Bytes: append([]byte(nil), decoderInst.Bytes...),
ExtensionWords: append([]uint16(nil), decoderInst.ExtensionWords...),
Metadata: convertMetadata(decoderInst.Metadata),
Metadata: decoderInst.Metadata,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve non-nil metadata slices

When decoding instructions with no immediate values, assigning the internal metadata directly retains its nil ImmediateValues slice; operand-less instructions such as NOP also retain nil Operands. The removed conversion always allocated zero-length slices, so JSON output changes from [] to null, which breaks consumers validating these fields as arrays and contradicts the stated no-behavior-change intent. Normalize these slices before returning the public instruction.

Useful? React with 👍 / 👎.

Comment thread public_types.go
// The structured decode types are defined once in the internal decoders package
// and re-exported here so callers have a single source of truth.
type (
DecodeMetadata = decoders.Metadata

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep DecodeMetadata as a public named type

For callers using reflection-based registries or identifiers, this alias changes reflect.TypeOf(m68kdasm.DecodeMetadata{}).Name() from DecodeMetadata to Metadata and its package path from github.com/jenska/m68kdasm to the internal decoder package. Such callers no longer recognize values produced after this ostensibly behavior-preserving change, so the public named-type identity needs to be retained or explicitly bridged.

Useful? React with 👍 / 👎.

- go.mod: go 1.26 -> 1.27; CI matrix 1.25.x -> 1.27.x
- replace uint32Ptr/int32Ptr helpers with the Go 1.26 new(expr) form
- range-over-int in formatRegisterList
- preallocate the instruction slice in DisassembleRange (>= len(data)/2)
- delete stray root go.yml (duplicate of .github/workflows/ci.yml, never
  executed by GitHub Actions)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jenska jenska changed the title Simplify decoders and remove duplicated public/internal types Simplify decoders, dedupe public/internal types, require Go 1.27 Sep 3, 2026
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jenska
jenska merged commit 422c0f4 into main Sep 3, 2026
1 check passed
@jenska
jenska deleted the simplify-decoders branch September 3, 2026 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant