Remove cells_frees duplicate storage from Frame#7351
Remove cells_frees duplicate storage from Frame#7351youknowone wants to merge 2 commits intoRustPython:mainfrom
Conversation
Cell/free variable objects were stored in both a separate `Box<[PyCellRef]>` (cells_frees field) and in the localsplus fastlocals array. Remove the redundant cells_frees field and access cell objects directly through localsplus, eliminating one Box allocation and N clone operations per frame creation.
📝 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin worktree-frame-datastack |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 97256480-8c7b-4237-bb71-6049292ef84a
📒 Files selected for processing (1)
crates/vm/src/frame.rs
| // Cell refs in fastlocals[nlocals..] are cleared by clear_locals_and_stack(). | ||
| unsafe { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 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.rsRepository: 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=50Repository: 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 -nRepository: 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 -nRepository: 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 -nRepository: 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.
🤖 Prompt for AI Agents
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.
1 participant
Cell/free variable objects were stored in both a separate
Box<[PyCellRef]>(cells_frees field) and in the localsplus fastlocals array. Remove the redundant cells_frees field and access cell objects directly through localsplus, eliminating one Box allocation and N clone operations per frame creation.Summary by CodeRabbit