GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Move some free standing functions to `ir::InstructionInfo` methods by ShaharNaveh · Pull Request #8209 · RustPython/RustPython Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR moves instruction sizing, location, predicate, and rewrite helpers onto ChangesInstructionInfo method migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
crates/codegen/src/ir.rs (1)
2682-2713: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three super-instruction arms into one call.
All three match arms differ only in the
super_opvalue while sharing thesplit_at_mut(1)+make_super_instruction(...)logic. Extract the differing opcode first, then call once. The first arm's nestedifis equivalent to matching(Some(Opcode::LoadFast), Some(Opcode::LoadFast))directly.As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
♻️ Proposed refactor
- match (block.instructions[i].instr.real_opcode(), nextop) { - (Some(Opcode::LoadFast), _) => { - if matches!(nextop, Some(Opcode::LoadFast)) { - let (inst1, rest) = block.instructions[i..].split_at_mut(1); - InstructionInfo::make_super_instruction( - &mut inst1[0], - &mut rest[0], - Opcode::LoadFastLoadFast.into(), - ); - } - } - - (Some(Opcode::StoreFast), Some(Opcode::LoadFast)) => { - let (inst1, rest) = block.instructions[i..].split_at_mut(1); - InstructionInfo::make_super_instruction( - &mut inst1[0], - &mut rest[0], - Opcode::StoreFastLoadFast.into(), - ); - } - - (Some(Opcode::StoreFast), Some(Opcode::StoreFast)) => { - let (inst1, rest) = block.instructions[i..].split_at_mut(1); - InstructionInfo::make_super_instruction( - &mut inst1[0], - &mut rest[0], - Opcode::StoreFastStoreFast.into(), - ); - } - - (_, _) => {} - } + let super_op = match (block.instructions[i].instr.real_opcode(), nextop) { + (Some(Opcode::LoadFast), Some(Opcode::LoadFast)) => { + Some(Opcode::LoadFastLoadFast) + } + (Some(Opcode::StoreFast), Some(Opcode::LoadFast)) => { + Some(Opcode::StoreFastLoadFast) + } + (Some(Opcode::StoreFast), Some(Opcode::StoreFast)) => { + Some(Opcode::StoreFastStoreFast) + } + (_, _) => None, + }; + if let Some(super_op) = super_op { + let (inst1, rest) = block.instructions[i..].split_at_mut(1); + InstructionInfo::make_super_instruction( + &mut inst1[0], + &mut rest[0], + super_op.into(), + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegen/src/ir.rs` around lines 2682 - 2713, Refactor the match in ir.rs that builds super-instructions so the repeated split_at_mut(1) and InstructionInfo::make_super_instruction call happens only once. In the match around block.instructions[i].instr.real_opcode(), first determine the super_op for the three supported pairs (LoadFast/LoadFast, StoreFast/LoadFast, StoreFast/StoreFast), then perform the shared mutation once using that opcode; the current nested if in the LoadFast arm should be treated as a direct (Some(Opcode::LoadFast), Some(Opcode::LoadFast)) case.Source: Coding guidelines
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 2682-2713: Refactor the match in ir.rs that builds
super-instructions so the repeated split_at_mut(1) and
InstructionInfo::make_super_instruction call happens only once. In the match
around block.instructions[i].instr.real_opcode(), first determine the super_op
for the three supported pairs (LoadFast/LoadFast, StoreFast/LoadFast,
StoreFast/StoreFast), then perform the shared mutation once using that opcode;
the current nested if in the LoadFast arm should be treated as a direct
(Some(Opcode::LoadFast), Some(Opcode::LoadFast)) case.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eb7380ae-bd2f-41ca-9d7c-3c0fce2b6daa
crates/codegen/src/ir.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…8209) * Move some free standing functions to methods * align tests * mark loads_const as const fn * coderabbit suggestion
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.