FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove `Reverse` bytecode by ShaharNaveh · Pull Request #6675 · RustPython/RustPython · GitHub

/ RustPython Public

Remove Reverse bytecode - #6675

Merged
youknowone merged 9 commits into
RustPython:mainfrom
ShaharNaveh:bytecode-remove-reverse
Jan 10, 2026
ConversationCommits (9)ChecksFiles changed
Merged

Remove Reverse bytecode#6675
youknowone merged 9 commits into
RustPython:mainfrom
ShaharNaveh:bytecode-remove-reverse

Conversation

ShaharNaveh commented Jan 9, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Chores

    • Removed a legacy bytecode instruction and its runtime handling, simplifying the instruction set and trimming related formatting/diagnostic paths.
    • Simplified opcode metadata/name generation logic by removing redundant code paths.
    • CI validation tightened to fail when generated metadata is out of date.

  • Breaking Changes

    • Potential bytecode compatibility impact for programs or artifacts that relied on the removed instruction; affected items should be regenerated or recompiled.

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

coderabbitai Bot commented Jan 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Removed the RustPython-only Reverse { amount: Arg<u32> } opcode variant from the Instruction enum, deleted its execution handling in the VM, and removed related stack-effect, disassembly/formatting, and dead-code paths; also fixed opcode-name generation and inverted a CI workflow check.

Changes

Cohort / File(s) Change Summary
Bytecode instruction definition
crates/compiler-core/src/bytecode.rs		 Removed Reverse { amount: Arg<u32> } from Instruction; removed its TryFrom<u8> mapping, stack-effect branch, and disassembly/formatting entry.
Frame execution handler
crates/vm/src/frame.rs		 Removed Reverse handling from ExecutingFrame::execute_instruction (stack slicing and in-place reversal logic deleted).
Opcode metadata & tooling
scripts/generate_opcode_metadata.py		 Removed redundant/alternative return paths in Opcode.cpython_name, consolidating name derivation to a single code path.
CI workflow
.github/workflows/ci.yaml		 Inverted the condition in the "Ensure Lib/_opcode_metadata is updated" step so the job fails when the working tree is dirty (non-empty git status --porcelain).

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐇 I nudged the opcodes, gave them a glance,

The Reverse hop has hopped its last dance.
Stacks now flow in straighter streams,
I twitch my nose and polish dreams.
Carrots for tidy, humming machines! 🎉

🚥 Pre-merge checks | ✅ 3 ✅ 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 and directly describes the main change—removing the Reverse bytecode operation from the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab43e94 and d879bf1.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml
🧰 Additional context used 🧠 Learnings (1) 📓 Common learnings 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When modifying bytecode instructions, perform a full clean build by running `rm -r target/debug/build/rustpython-* && find . | grep -E '\.pyc$' | xargs rm -r`
🔇 Additional comments (1)
.github/workflows/ci.yaml (1)

334-339: LGTM! Correct enforcement of metadata synchronization.

The inverted condition now properly fails the CI when the opcode metadata generation produces uncommitted changes, ensuring developers commit updated metadata alongside opcode definition changes. This is a standard best practice for keeping generated files synchronized with source code.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

youknowone reviewed Jan 9, 2026
View reviewed changes
Comment thread crates/compiler-core/src/bytecode.rs Outdated
ShaharNaveh and others added 2 commits January 9, 2026 17:22
Update crates/compiler-core/src/bytecode.rs
2cf54f8 
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Gen
ad05793
ShaharNaveh mentioned this pull request Jan 9, 2026
Merged

github-actions Bot commented Jan 10, 2026

Copy link
Copy Markdown
Contributor

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin bytecode-remove-reverse

ShaharNaveh commented Jan 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all. Please pull the latest changes before pushing again:

git pull origin bytecode-remove-reverse

Yay #6677 works

youknowone commented Jan 10, 2026

Copy link
Copy Markdown
Member

Great! The message looks like need to be changed. it is not by fmt --all

youknowone merged commit 6fe0598 into RustPython:main Jan 10, 2026
13 checks passed
ShaharNaveh added a commit to ShaharNaveh/RustPython that referenced this pull request Jan 10, 2026
Remove Reverse bytecode (RustPython#6675)
d448753 
* Remove `Reverse` bytecode

* Update crates/compiler-core/src/bytecode.rs

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>

* Gen

* Remove Reverse

* Auto-format: cargo fmt --all

* Revert comment

* Remove debug code

* Fix CI

---------

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 15, 2026
Remove Reverse bytecode (RustPython#6675)
dc39bd6 
* Remove `Reverse` bytecode

* Update crates/compiler-core/src/bytecode.rs

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>

* Gen

* Remove Reverse

* Auto-format: cargo fmt --all

* Revert comment

* Remove debug code

* Fix CI

---------

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL