Skip to content

One match plan for C and JS emitters - #986

Closed
jkbennitt wants to merge 2 commits into
bendlang:mainfrom
AppSprout-dev:cursor/match-plan-emitters-4b29
Closed

jkbennitt wants to merge 2 commits into
bendlang:mainfrom
AppSprout-dev:cursor/match-plan-emitters-4b29

Conversation

@jkbennitt

@jkbennitt jkbennitt commented Sep 22, 2026

Copy link
Copy Markdown

Summary

The C and JS match emitters decide a Nat chain, a 32-bit word's leaves, and a constant table with the same prelude. match_plan in bend2/comp.ts computes that plan once. Each lane still builds its own arms: C keeps its Levels and the scrutinee hold, JS keeps field extraction and the IO.OP default.

bend2/bend.ts is untouched.

Test plan

  • bun gates/repo.ts — PASS 46 / 46
  • Local mirror of gates/test.ts (checkup aggregators, clang -O3, 5 s alarm) — PASS 1427 / 1427
  • Cluster gate (not run here; no minis)

Nat chains, word leaves, and constant tables are decided once, and each lane still builds its own arms.

Co-authored-by: Jason <jkbennitt@users.noreply.github.com>
@Derenash Derenash self-assigned this Sep 22, 2026

@nicolas-abril nicolas-abril left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, this is a clean one. I emitted C and JS for every runnable test on main and on this branch and the outputs are byte-identical, so the refactor is verified behavior-preserving.

Two small things before merging:

  1. match_plan has no declared return type. It would be the only TypeScript function in comp.ts without one (the untyped functions are the JS runtime ones inside the runtime string). Please write the return type out.

Optional: on the constant-table path the C lane now computes total and mat_arms(x) it never uses. Compile-time only and harmless, so leave it unless you see a tidy way around it.

Co-authored-by: Jason <jkbennitt@users.noreply.github.com>
@jkbennitt

Copy link
Copy Markdown
Author

Thanks — pushed MatchPlan as an explicit return type on match_plan (44ebe58).

On the optional unused total / mat_arms work on the C constant-table path: I tried lifting those out of the shared plan so the early return skips them. It only saved a couple of lines and put the lookups back in both emitters for every non-table match, so I left the shared plan as-is. Happy to revisit if you’d rather they stay lazy.

@jkbennitt jkbennitt changed the title One match plan for C and JS emitters (negative lines) One match plan for C and JS emitters Sep 22, 2026
@jkbennitt

Copy link
Copy Markdown
Author

Quick honesty pass on the title: after adding the explicit MatchPlan return type Nicolas asked for, this PR is no longer a net line delete (+31 / −19). Left the type in; retitled to drop the “negative lines” claim. The shared plan and byte-identical emit still stand.

@nicolas-abril

nicolas-abril commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks a lot for this, Jason, and for how quickly you picked up the review notes!

I ended up folding this into a larger cleanup of comp.ts that just landed on main in 2665926, and it covers the same ground in a different shape, so I'm closing this one as superseded. Your observation that both match emitters derive the same prelude was exactly right, and it's what started that part of the change. Instead of one plan object, the pieces went to where each one belongs: emit_tab now returns the table read for its lane directly, the Nat and word row producers check their own datatype, and a small mat_ctrs builds the constructor arms plus the default. That way neither lane needs a named plan type, and the constant-table path no longer computes arms it never uses, which covers both points from the review.

Nothing wrong with your version: it was clean, verified byte-identical, and a good call. It just happened to overlap with a broader rewrite of those same functions. Contributions like this are really appreciated, and I hope to see more from you!

Note: this reply was written by an AI after it reported the PR to me and I made the decision. If anything here is wrong, reply and I will review it myself.

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.

4 participants