Remove vstack code from builder and tests#63
Conversation
Deletes all vstack infrastructure: the `vstack` option from `Options`, the `_vstack` field and methods from `BuildCtx`, the `__sync_vstack` helper and all its call sites, and all commented-out vstack branches in the stack push/pop helpers and `push` opcode. Also cleans up `use_vstack` CLI arguments in jetdbg and the disabled vstack test variant in `rom_tests!`. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK
|
Review changes with Changed Files
|
Summary of ChangesHello @tcrypt25519, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant code cleanup by completely removing the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully removes the vstack (virtual stack) infrastructure from the jet crate. This includes removing the vstack option from the configuration, removing the _vstack field and associated methods from the build context, and cleaning up all call sites and commented-out code related to the virtual stack. This refactoring simplifies the compiler's internal state and removes dead or disabled code, improving maintainability. The changes are consistent across the modified files and follow the project's structure.
There was a problem hiding this comment.
Pull request overview
Removes the (currently disabled) virtual stack (“vstack”) scaffolding from Jet’s EVM IR builder and associated tests/CLI plumbing, simplifying the build context and stack helpers to always use the runtime stack.
Changes:
- Removes
vstackfrombuilder::env::Optionsand updates related call sites in the ROM test harness andjetdbg. - Deletes
BuildCtxvstack storage/accessors and removes vstack sync helper/call sites in the builder. - Cleans up commented-out vstack branches in stack push/pop helpers and the ROM test macro.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/jet/tests/roms/mod.rs | Drops the vstack test variant and simplifies _test_rom_body to no longer accept/use a vstack flag. |
| crates/jet/src/builder/ops.rs | Removes __sync_vstack and all commented vstack branches; stack helpers now always call runtime stack builtins. |
| crates/jet/src/builder/env.rs | Removes the vstack option/field and updates Options::new signature accordingly. |
| crates/jet/src/builder/contract.rs | Removes vstack storage from BuildCtx and eliminates pre-branch vstack sync. |
| crates/jet/src/bin/jetdbg.rs | Removes the --use-vstack CLI args and updates Options::new invocation. |
Strips the double-underscore convention from all private helpers in ops.rs (e.g. __stack_pop_2 → stack_pop_2, __call_stack_push_i256 → call_stack_push_i256) and from the two pub(crate) entry points (__build_return → build_return, __invalid_jump_return → invalid_jump_return). Reorders the file so all pub(crate) opcode implementations appear first, followed by a clearly-marked "Private helpers" section. Updates the one external call site in contract.rs. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK
Options::new takes (mode, emit_llvm, assert); remove the stray `false` that was inserted before the `assert` argument. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK
Dead code with no call sites; flagged by clippy -D warnings. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK
The vstack infrastructure and Options flag were removed in this PR. Update architecture.md, architecture-notes.md, and manifesto.md to: - Drop the BuildCtx _vstack field from struct snippets - Rename the "Hybrid Stack Model" section to "Real Stack Model" and remove the vstack-enabled code example and planned-optimization prose - Remove __sync_vstack from the control flow pattern snippet - Update helper function names to match the renamed (no-__ prefix) API - Remove vstack from Known TODOs, Future Opportunities, Suggested Next Steps, Options description, and the Glossary - Remove the "Virtual Stack Optimization" and "The Virtual Stack" caution-zone sections from the manifesto https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK
* Remove vstack code from builder and tests Deletes all vstack infrastructure: the `vstack` option from `Options`, the `_vstack` field and methods from `BuildCtx`, the `__sync_vstack` helper and all its call sites, and all commented-out vstack branches in the stack push/pop helpers and `push` opcode. Also cleans up `use_vstack` CLI arguments in jetdbg and the disabled vstack test variant in `rom_tests!`. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK * Remove __ prefix from private identifiers and move them below public fns Strips the double-underscore convention from all private helpers in ops.rs (e.g. __stack_pop_2 → stack_pop_2, __call_stack_push_i256 → call_stack_push_i256) and from the two pub(crate) entry points (__build_return → build_return, __invalid_jump_return → invalid_jump_return). Reorders the file so all pub(crate) opcode implementations appear first, followed by a clearly-marked "Private helpers" section. Updates the one external call site in contract.rs. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK * chore: CI change 🤖 * Fix extra argument in invalid_opcode test Options::new takes (mode, emit_llvm, assert); remove the stray `false` that was inserted before the `assert` argument. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK * Remove unused invalid_jump_return function Dead code with no call sites; flagged by clippy -D warnings. https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK * Remove vstack references from docs The vstack infrastructure and Options flag were removed in this PR. Update architecture.md, architecture-notes.md, and manifesto.md to: - Drop the BuildCtx _vstack field from struct snippets - Rename the "Hybrid Stack Model" section to "Real Stack Model" and remove the vstack-enabled code example and planned-optimization prose - Remove __sync_vstack from the control flow pattern snippet - Update helper function names to match the renamed (no-__ prefix) API - Remove vstack from Known TODOs, Future Opportunities, Suggested Next Steps, Options description, and the Glossary - Remove the "Virtual Stack Optimization" and "The Virtual Stack" caution-zone sections from the manifesto https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK --------- Co-authored-by: Claude <noreply@anthropic.com>
Deletes all vstack infrastructure: the
vstackoption fromOptions,the
_vstackfield and methods fromBuildCtx, the__sync_vstackhelper and all its call sites, and all commented-out vstack branches
in the stack push/pop helpers and
pushopcode. Also cleans upuse_vstackCLI arguments in jetdbg and the disabled vstack testvariant in
rom_tests!.https://claude.ai/code/session_015PYMujjEHLtp7nkvCHF5PK