Resume opcode to hold ResumeType#7465
Conversation
📝 WalkthroughWalkthroughRefactored the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
@youknowone I'm thinking to create a derive macro called: RustPython/crates/compiler-core/src/bytecode/oparg.rs Lines 265 to 277 in b11b8e6 and RustPython/crates/compiler-core/src/bytecode/oparg.rs Lines 575 to 585 in b11b8e6 (we have more places) Also with generate the lmk what you think:) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode/oparg.rs (1)
313-317: Consider showing variant names in Display for better diagnostics.The current
Displayimpl shows only the numeric value. For disassembly output, showing variant names could improve readability.🔧 Optional enhancement for Display
impl core::fmt::Display for ResumeType { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - u32::from(*self).fmt(f) + match self { + Self::AtFuncStart => write!(f, "0 (AtFuncStart)"), + Self::AfterYield => write!(f, "1 (AfterYield)"), + Self::AfterYieldFrom => write!(f, "2 (AfterYieldFrom)"), + Self::AfterAwait => write!(f, "3 (AfterAwait)"), + Self::Other(v) => write!(f, "{v}"), + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 313 - 317, The Display impl for ResumeType currently prints only its numeric value (u32::from(*self).fmt(f)); change it to print human-friendly variant names for diagnostics by matching on ResumeType (in the impl core::fmt::Display for ResumeType, fn fmt) and write the variant identifier strings (e.g., "None", "Yield", etc.) to the formatter, optionally falling back to the numeric value for unknown/other variants to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 313-317: The Display impl for ResumeType currently prints only its
numeric value (u32::from(*self).fmt(f)); change it to print human-friendly
variant names for diagnostics by matching on ResumeType (in the impl
core::fmt::Display for ResumeType, fn fmt) and write the variant identifier
strings (e.g., "None", "Yield", etc.) to the formatter, optionally falling back
to the numeric value for unknown/other variants to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a841bb7a-52f4-4af9-85c5-31e89e5a26d1
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/oparg.rs
|
@ShaharNaveh sounds good to me. Could you show me an example how it will look like once the macro is introduced? |
I was thinking something like: #[newtype_oparg]
struct VarNum(u32);
#[newtype_oparg]
enum SpecialMethod {
/// `__enter__` for sync context manager
#[oparg(display = "__enter__")]
Enter = 0,
/// `__exit__` for sync context manager
#[oparg(display = "__exit__")]
Exit = 1,
...
}
#[newtype_oparg]
enum ResumeType {
AfterYield = 1
AfterYieldFrom = 2,
#[oparg(catch_all)]
Other(u32)
} |
|
@youknowone loks good? |
Summary by CodeRabbit