Fix recently broken Deparse #23229
Fix recently broken Deparse #23229
Conversation
v5.41.7-43-gcdbed2a40e introduced the OP_SUBSTR_LEFT op,
which is an optimised version of OP_SUBSTR for when the offset
is zero and the replacement string is missing or ''.
Unfortunately the deparsing for this OP missed out the closing
parenthesis when the replacement string wasn't present; i.e.:
substr($lex, 0, 4);
deparsed as:
substr($lex, 0, 4;
The fix is trivial.
Add a couple of recently-added test files to
Porting/deparse-skips.txt
These two test files have
use utf8;
"some string with accented chars"
and the accented char gets deparsed wrongly at the moment.
So skip those test files under
cd t; ./TEST -deparse
|
Quick review suggests (a) branch in p.r. passes newly furnished tests; (b) blead fails on same tests. Hence, +1 to merge. However, at this point in the dev cycle, additional eyeballs welcome. |
|
@richardleach, my hunch is that one of your commits is of concern here. Please review. |
| my $val = 'substr(' . $self->deparse($op->first->sibling, $cx) | ||
| . ', 0, ' . $self->deparse($op->first->sibling->sibling->sibling, $cx) | ||
| . ( (($op->private & 7) == 3) ? '' : ", '')" ); | ||
| . ( (($op->private & 7) == 3) ? ')' : ", '')" ); |
There was a problem hiding this comment.
Could even pull the common tail out:
| . ( (($op->private & 7) == 3) ? ')' : ", '')" ); | |
| . ( (($op->private & 7) == 3) ? '' : ", ''" ) | |
| . ')'; |
There was a problem hiding this comment.
I wanted to make the change as absolutely minimal as possible, which is why I didn't factor out the common ')'.
|
Inspection of I hacked up a copy of I then built perl at cdbed2a and ran the test file through the harness. It FAILed, as expected, the flaw having just been introduced. I have built the branch represented in this p.r. on both Linux and FreeBSD and gotten PASSes; merging into blead now. |
Deparsing of 'substr($lex, 0, N)' was broken in 5.41.8. The two commits in this PR fix it and update deparse-skips.txt to skip a couple of recently-added tests. I propose that this PR be merged now, in time for 5.42.0.