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 — Remove cells_frees duplicate storage from Frame by youknowone · Pull Request #7351 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughFrame's cell and free variable storage refactored from a separate Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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)
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.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin worktree-frame-datastack |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Actionable comments posted: 1
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 743-744: Rename the misleading method clear_stack_and_cells() to
clear_stack() and update its docstring to state it only clears the value stack
(delegating cell/free clearing to clear_locals_and_stack()); inside the function
keep calling stack_clear() as before, and update all callsites and references
(including any uses alongside clear_locals_and_stack()) to the new name to
maintain behavior; ensure public API/export names are adjusted and tests/uses
compile.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 97256480-8c7b-4237-bb71-6049292ef84a
crates/vm/src/frame.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Cell refs in fastlocals[nlocals..] are cleared by clear_locals_and_stack(). | ||
| unsafe { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash # First, find the clear_stack_and_cells method definition and all callsites rg -nP --type=rust -B2 -A8 'fn clear_stack_and_cells' crates/vm/src/frame.rs
Repository: RustPython/RustPython
Length of output: 607
🏁 Script executed:
#!/bin/bash # Find all callsites of clear_stack_and_cells rg -nP --type=rust '\bclear_stack_and_cells\s*\(' --max-count=50
Repository: RustPython/RustPython
Length of output: 279
🏁 Script executed:
#!/bin/bash # Look at lines 743-744 and surrounding context to understand the comment sed -n '735,755p' crates/vm/src/frame.rs | cat -n
Repository: RustPython/RustPython
Length of output: 1210
🏁 Script executed:
#!/bin/bash # Check the callsite at crates/vm/src/builtins/frame.rs:702 sed -n '695,710p' crates/vm/src/builtins/frame.rs | cat -n
Repository: RustPython/RustPython
Length of output: 554
🏁 Script executed:
#!/bin/bash # Also check context around line 752 in frame.rs for the other callsite sed -n '748,760p' crates/vm/src/frame.rs | cat -n
Repository: RustPython/RustPython
Length of output: 704
Rename clear_stack_and_cells to clarify misleading semantics.
The method name and docstring claim it clears cell/free references, but the implementation only calls stack_clear(). The comment at line 743 explicitly states cells are cleared by clear_locals_and_stack() instead. Both callsites handle cells correctly (either cleared before the call or by the wrapper method), so no correctness issue exists, but renaming to clear_stack() would eliminate confusion about the method's actual responsibilities.
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/frame.rs` around lines 743 - 744, Rename the misleading method
clear_stack_and_cells() to clear_stack() and update its docstring to state it
only clears the value stack (delegating cell/free clearing to
clear_locals_and_stack()); inside the function keep calling stack_clear() as
before, and update all callsites and references (including any uses alongside
clear_locals_and_stack()) to the new name to maintain behavior; ensure public
API/export names are adjusted and tests/uses compile.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.