Skip to content

Rework multibyte opcode support #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 24, 2025
Merged

Rework multibyte opcode support #319

merged 6 commits into from
May 24, 2025

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented May 24, 2025

  • Removed the Opcode parameter from Efn -- when there are multibyte opcode this becomes confusing, plus the single byte Opcode can be easily fetched as machine.code()[position].
  • Added MultiEtable, where each item can itself be another etable. During evaluation the extra position change will be added by the "parent" node.

@sorpaas sorpaas requested a review from Copilot May 24, 2025 15:02
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors opcode dispatch to support multibyte opcodes by removing the explicit Opcode parameter from evaluation functions and introducing composable etable types (Single, Chained, and MultiEtable).

  • Remove the Opcode argument from all eval function signatures and bypass single‐byte tables.
  • Add Single for single‐function etables, Chained for combining two etables, and MultiEtable for nested opcode tables.
  • Update tests and interpreter core to use the new etable API.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/eip_6780.rs Updated tests to use Single::new and Chained etable API
src/standard/gasometer/mod.rs Removed old opcode parameter and changed NextEtable to NoAction
jsontests/src/run.rs Updated JSON tests to use Single::new and Chained
interpreter/tests/usability.rs Added MultiEtable usage and refactored wrap closures
interpreter/src/interpreter/etable.rs Changed interpreter loop to call etable.eval(position)
interpreter/src/eval/mod.rs Removed unused _opcode parameters from all eval functions
interpreter/src/etable.rs Introduced Single, Chained, MultiEfn, MultiEtable, and updated EtableSet API
interpreter/src/error/exit.rs Deleted obsolete UnknownEtable variant

MultiEfn::Node(n) => {
let nextpos = position + 1;
if nextpos >= machine.code.len() {
return Control::Exit(ExitException::PCUnderflow.into());
Copy link
Preview

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

Throwing PCUnderflow when the next position exceeds the code length seems incorrect; out-of-bounds positions should raise a PCOverflow exception instead.

Suggested change
return Control::Exit(ExitException::PCUnderflow.into());
return Control::Exit(ExitException::PCOverflow.into());

Copilot uses AI. Check for mistakes.

@sorpaas sorpaas merged commit 7f1fe58 into master May 24, 2025
3 checks passed
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