-
Notifications
You must be signed in to change notification settings - Fork 605
Clear all variable pad slots after multi-variable foreach loop
#24034
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9d73bc4 to
6ec3cbe
Compare
|
Oh dear. One failing test on macOS, dist/threads complains about a panic to copy freed scalar. There's a chance that actually is related to my change. |
|
Though, looking further at the test ( I'll rerun it and see if it happens again; it could be a spurious unrelated issue. |
|
Well that's slightly awkward. Just rerunning the test made it pass. I'm a little nervous about that because it doesn't explain why the test previously failed, but I've looked up and down the code and I really can't see how it relates to multivariable-foreach and also the failing test file runs reliably without upsetting |
inline.h
Outdated
|
|
||
| cx->blk_loop.itervar_u.svp = (SV**)itervarp; | ||
| cx->blk_loop.itersave = itersave; | ||
| cx->blk_loop.additional_padvars = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perl_cx_dup also sets blk_loop.itersave. Would it need to initialize blk_loop.additional_padvars, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that's necessary. The Copy() operation will copy the value of this field from old to new; then various fixups happen afterwards to adjust pointer-like things to point at nested cloned structures. This field just stores an integer, so needs no further treatment.
inline.h
Outdated
| cx->blk_loop.itersave = NULL; | ||
| SvREFCNT_dec(cursv); | ||
| } | ||
| if (cx->cx_type & CXp_FOR_PAD && cx->blk_loop.additional_padvars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could cx->cx_type & CXp_FOR_PAD be written as CxPADLOOP(cx)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it can :)
6ec3cbe to
1654805
Compare
We need to remember to clear all the pad slots related to iteration variables, which means some number of additional ones after the main "itervar" when doing multi-variable iteration. This ensures we don't retain references or large scalar values longer than necessary.
1654805 to
0a46b1b
Compare
We need to remember to clear all the pad slots related to iteration variables, which means some number of additional ones after the main "itervar" when doing multi-variable iteration. This ensures we don't retain references or large scalar values longer than necessary.
I'll write a perldelta for this + the other recent fixes for foreach loops once they're all in, before the next point release.