Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Perl: porters

[PATCH] Perl RT#3105 Mark elements of constant range as read only

  

 
Perl porters RSS feed   Index | Next | Previous | View Threaded


ikegami at adaelis

Nov 24, 2009, 4:52 PM

Post #1 of 18 (600 views)
Permalink
[PATCH] Perl RT#3105 Mark elements of constant range as read only
Hi,

This makes 1..5 no different than 1,2,3,4,5 in terms of functionality, and
avoids some very subtle bugs.

- ELB
Attachments: 0001-Mark-elements-of-constant-range-as-read-only.-3105.patch (3.24 KB)


abigail at abigail

Nov 25, 2009, 12:26 AM

Post #2 of 18 (582 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Tue, Nov 24, 2009 at 07:52:38PM -0500, Eric Brine wrote:
> Hi,
>
> This makes 1..5 no different than 1,2,3,4,5 in terms of functionality, and
> avoids some very subtle bugs.
>


It does however have the potential to break code.


Abigail


ikegami at adaelis

Nov 25, 2009, 8:17 AM

Post #3 of 18 (584 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 3:26 AM, Abigail <abigail [at] abigail> wrote:

> On Tue, Nov 24, 2009 at 07:52:38PM -0500, Eric Brine wrote:
> > Hi,
> >
> > This makes 1..5 no different than 1,2,3,4,5 in terms of functionality,
> and
> > avoids some very subtle bugs.
> >
>
>
> It does however have the potential to break code.
>

Yes. Specifically, it'll break code (noisily) that modifies what Perl treats
as constants. I even had to fix such a bug in t/op/mydef.t. This is what the
submitter and the responders requested. If you'd rather I do it differently,
I'm up to giving it a go.


ikegami at adaelis

Nov 25, 2009, 10:04 AM

Post #4 of 18 (579 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 11:17 AM, Eric Brine <ikegami [at] adaelis> wrote:

> On Wed, Nov 25, 2009 at 3:26 AM, Abigail <abigail [at] abigail> wrote:
>
>> It does however have the potential to break code.
>>
>
> Yes. Specifically, it'll break code (noisily) that modifies what Perl
> treats as constants. I even had to fix such a bug in t/op/mydef.t. This is
> what the submitter and the responders requested. If you'd rather I do it
> differently, I'm up to giving it a go.
>

Since the bug is due to an optimisation, I figured it would be worthwhile to
benchmark the usefulness of the optimisation.

There's almost no difference for small lists. For long lists, the
optimisation results in a substantial difference in performance.

$ git diff
diff --git a/op.c b/op.c
index d4f6fb3..32fa2b4 100644
--- a/op.c
+++ b/op.c
@@ -2590,6 +2590,8 @@ S_gen_constant_list(pTHX_ register OP *o)
register OP *curop;
const I32 oldtmps_floor = PL_tmps_floor;

+ return o;
+
list(o);
if (PL_parser && PL_parser->error_count)
return o; /* Don't attempt to run with errors */

$ cat a.pl
use Time::HiRes qw( time );
my $N = $ARGV[0] || 2000;
my $s_time = time;
my @a = map { my $p=$_; map { "$p:$_" } 1..$N } 1..$N;
my $e_time = time;
print($e_time - $s_time, "\n");


Blead (at 789c461534f3eb0346447f8127786b7da3017f6c):

$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 20 ; done
0.000880002975463867
0.000934123992919922
0.000874042510986328
0.000941991806030273
0.000870943069458008
0.000852108001708984

avg: 0.000892202059427897

$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 2000 ; done
6.18947696685791
6.17325901985168
6.38008713722229
6.51624393463135
6.2569899559021
6.23375201225281

avg: 6.29163483778636


Blead with optimisation removed:

$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 20 ; done
0.000860929489135742
0.000841140747070312
0.000859975814819336
0.000878810882568359
0.000877141952514648
0.000889062881469727

avg: 0.000867843627929687

$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 2000 ; done
9.09057521820068
8.82025122642517
9.41165494918823
8.80950093269348
9.01842403411865
8.80255198478699

avg: 8.99215972423553


- ELB


abigail at abigail

Nov 25, 2009, 12:33 PM

Post #5 of 18 (575 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 11:17:04AM -0500, Eric Brine wrote:
> On Wed, Nov 25, 2009 at 3:26 AM, Abigail <abigail [at] abigail> wrote:
>
> > On Tue, Nov 24, 2009 at 07:52:38PM -0500, Eric Brine wrote:
> > > Hi,
> > >
> > > This makes 1..5 no different than 1,2,3,4,5 in terms of functionality,
> > and
> > > avoids some very subtle bugs.
> > >
> >
> >
> > It does however have the potential to break code.
> >
>
> Yes. Specifically, it'll break code (noisily) that modifies what Perl treats
> as constants. I even had to fix such a bug in t/op/mydef.t. This is what the
> submitter and the responders requested. If you'd rather I do it differently,
> I'm up to giving it a go.


I've no suggestion to do it differently. I'm just pointing out the patch
comes with a price - it may break code. I've no opinion on whether that
price is one we want to pay.

I've written code in past like this:

for (1 .. 3) {
$_ .= "foo";
say;
}
__END__
1foo
2foo
3foo

which just DWIM. I'd be surprised if others haven't done so either,
be it on purpose, or by accident.

OTOH, I had code that broke when

map {s/.//; $_} "ab", "bc", "cd";

no longer was possible, and that breakage didn't stop progress from happening.

But that was 10 years ago, and peoples opinion on backwards compatability
have changed.


I've never been happy with the restriction of not being able to modify
values - the fact the values are marked read-only is an implementation
issue - and I prefer programmers not to be hindered by implementation
issues.


Abigail


xdaveg at gmail

Nov 25, 2009, 1:01 PM

Post #6 of 18 (572 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 3:33 PM, Abigail <abigail [at] abigail> wrote:
> I've written code in past like this:
>
>    for (1 .. 3) {
>        $_ .= "foo";
>        say;
>    }

Yes, but if that was (1,2,3), it would fail. E.g., currently:

$ perl -E 'for (1, 2, 3) { $_.="foo"; say }'
Modification of a read-only value attempted at -e line 1.

$ perl -E 'for (1 .. 3) { $_.="foo"; say }'
1foo
2foo
3foo

So I think making constant *range* act like a constant *list* is
"fixing a bug" not "breaking a feature".

-- David


ikegami at adaelis

Nov 25, 2009, 2:29 PM

Post #7 of 18 (577 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 3:33 PM, Abigail <abigail [at] abigail> wrote:

> I've no suggestion to do it differently. I'm just pointing out the patch
> comes with a price - it may break code. I've no opinion on whether that
> price is one we want to pay.
>

Since fixing the bug can have negative effects, we could deprecate in 5.12
(by adding Set magic to the returned values), fix in 5.14.


rgs at consttype

Nov 26, 2009, 12:37 AM

Post #8 of 18 (579 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
2009/11/25 David Golden <xdaveg [at] gmail>:
> On Wed, Nov 25, 2009 at 3:33 PM, Abigail <abigail [at] abigail> wrote:
>> I've written code in past like this:
>>
>>    for (1 .. 3) {
>>        $_ .= "foo";
>>        say;
>>    }
>
> Yes, but if that was (1,2,3), it would fail.  E.g., currently:
>
>  $ perl -E 'for (1, 2, 3) { $_.="foo"; say }'
>  Modification of a read-only value attempted at -e line 1.
>
>  $ perl -E 'for (1 .. 3) { $_.="foo"; say }'
>  1foo
>  2foo
>  3foo
>
> So I think making constant *range* act like a constant *list* is
> "fixing a bug" not "breaking a feature".

I'd rather have it the other way around : make the for(1,2,3) variant
work, with some sort of copy-on-write.


nick at ccl4

Nov 26, 2009, 2:48 AM

Post #9 of 18 (571 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Thu, Nov 26, 2009 at 09:37:14AM +0100, Rafael Garcia-Suarez wrote:
> 2009/11/25 David Golden <xdaveg [at] gmail>:

> > So I think making constant *range* act like a constant *list* is
> > "fixing a bug" not "breaking a feature".
>
> I'd rather have it the other way around : make the for(1,2,3) variant
> work, with some sort of copy-on-write.

But currently C<for> is defined as aliasing the iterator variable to each item
of the list in turn. Scalar literals are (currently) constants. Would you
envisage this change being for C<for> only? Or for all literals, so that
this would no longer be an error:

$ perl -le 'sub a { $_[0]++ }; a(1)'
Modification of a read-only value attempted at -e line 1.

Nicholas Clark


abigail at abigail

Nov 26, 2009, 2:52 AM

Post #10 of 18 (574 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 04:01:16PM -0500, David Golden wrote:
> On Wed, Nov 25, 2009 at 3:33 PM, Abigail <abigail [at] abigail> wrote:
> > I've written code in past like this:
> >
> >    for (1 .. 3) {
> >        $_ .= "foo";
> >        say;
> >    }
>
> Yes, but if that was (1,2,3), it would fail. E.g., currently:
>
> $ perl -E 'for (1, 2, 3) { $_.="foo"; say }'
> Modification of a read-only value attempted at -e line 1.
>
> $ perl -E 'for (1 .. 3) { $_.="foo"; say }'
> 1foo
> 2foo
> 3foo
>
> So I think making constant *range* act like a constant *list* is
> "fixing a bug" not "breaking a feature".


I probably missed it, but which bug is being fixed by forbidding to
modify the iterator in C<< for $iterator (RANGE) >>?


I've always considered C<< for (1 .. 3) { ... } >> to be more equivalent to
C<< for (my $x = 1; $x <= 3; $x ++) {local $_ = $x; ... } >> than to
C<< for (1, 2, 3) { ... } >>, ever since the range was no longer expanded to
a list at compile time. (5.004?)



Abigail


Eirik-Berg.Hanssen at allverden

Nov 26, 2009, 8:06 AM

Post #11 of 18 (567 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
Abigail <abigail [at] abigail> writes:

> I probably missed it, but which bug is being fixed by forbidding to
> modify the iterator in C<< for $iterator (RANGE) >>?

If nothing else, the bug that arises when someone replaces that
C<for> with a C<map> (minimal example):

perl -e 'sub a { print($_++) for 1..3; print $/ } a;a;a'
123
123
123

perl -e 'sub a { map print($_++), 1..3; print $/ } a;a;a'
123
234
345


Eirik
--
"[..] the buzz about Ruby and Rails is the sound of a bunch of
Java programmers finally discovering how cool Perl is."
-- Piers Cawley


Eirik-Berg.Hanssen at allverden

Nov 26, 2009, 8:16 AM

Post #12 of 18 (572 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
Eirik Berg Hanssen <Eirik-Berg.Hanssen [at] allverden> writes:

> Abigail <abigail [at] abigail> writes:
>
>> I probably missed it, but which bug is being fixed by forbidding to
>> modify the iterator in C<< for $iterator (RANGE) >>?
>
> If nothing else, the bug that arises when someone replaces that
> C<for> with a C<map> (minimal example):
>
> perl -e 'sub a { print($_++) for 1..3; print $/ } a;a;a'
> 123
> 123
> 123
>
> perl -e 'sub a { map print($_++), 1..3; print $/ } a;a;a'
> 123
> 234
> 345

Or even, as I find upon reading ikegami's recent recent additions to
the RT ticket:

perl -e 'sub a { print($_++) for 1..3, @x; print $/ } a;a;a'
123
234
345

All I did was add C<, @x>, and suddenly my little example subroutine
is self-modifying. Stateful. Whatever. Point is, I didn't ask for it.

Or did I? :)


Eirik
--
"So this is the Sword of Immortality? Huh?
What's it doin' in a CRYPT?!"
--- John S. Novak, III, quoting an unnamed player


ikegami at adaelis

Nov 26, 2009, 9:32 AM

Post #13 of 18 (573 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Thu, Nov 26, 2009 at 5:52 AM, Abigail <abigail [at] abigail> wrote:

> I probably missed it, but which bug is being fixed by forbidding to
> modify the iterator in C<< for $iterator (RANGE) >>?
>

C<< for $x (EXPR..EXPR) >> is special (optimised). It *doesn't* use the
range operator. It shouldn't be affected by the patch. (I'll test when I
can.) If we actually use the range operator, you'll see the problem if you
repeat the statment containing the range. For example,

for (1..2) {
for (1 .. 3,@x) {
$_ .= "foo";
say;
}
}

and

for (1..2) {
map {
$_ .= "foo";
say
} 1..3;
}

both output

1foo
2foo
3foo
1foofoo
2foofoo
3foofoo


ikegami at adaelis

Nov 26, 2009, 10:38 AM

Post #14 of 18 (570 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Thu, Nov 26, 2009 at 3:37 AM, Rafael Garcia-Suarez <rgs [at] consttype>wrote:

> I'd rather have it the other way around : make the for(1,2,3) variant
> work, with some sort of copy-on-write.
>

I'm going to worry about CONST..CONST first.

Behaviour in 5.12 and earlier:
CONST..CONST returns something that can be modified, but that you're not
allowed to modify.

What the bug report requests and what the patch provides:
CONST..CONST returns something that can't modified.

What I'd like:
CONST..CONST to return something that you can modify.

Means of achieving this:
Remove the optimisation that computes the list at compile-time.

Any other faster means?
I don't know. You mentioned copy on write. Is there an existing mechanism
for that?


ikegami at adaelis

Nov 26, 2009, 11:03 AM

Post #15 of 18 (573 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
n Thu, Nov 26, 2009 at 12:32 PM, Eric Brine <ikegami [at] adaelis> wrote:

> On Thu, Nov 26, 2009 at 5:52 AM, Abigail <abigail [at] abigail> wrote:
>
>> I probably missed it, but which bug is being fixed by forbidding to
>> modify the iterator in C<< for $iterator (RANGE) >>?
>>
>
> C<< for $x (EXPR..EXPR) >> is special (optimised). It *doesn't* use the
> range operator. It shouldn't be affected by the patch. (I'll test when I
> can.)
>

Confirmed. This patch does NOT affect C<< for (EXPR..EXPR) >>.

$ perl -Ilib -E'for (1..2) { for (1..3) { ++$_; say } }'
2
3
4
2
3
4

Those loops are optimised into counting loops. The range never gets
flattened. Other patterns are affected:

$ perl -Ilib -E'for (1..2) { for (1..3,@x) { ++$_; say } }'
Modification of a read-only value attempted at -e line 1.

$ perl -Ilib -E'for (1..2) { for (reverse 1..3) { ++$_; say } }'
Modification of a read-only value attempted at -e line 1.


zefram at fysh

Nov 26, 2009, 12:33 PM

Post #16 of 18 (566 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
Eric Brine wrote:
>What the bug report requests and what the patch provides:
>CONST..CONST returns something that can't modified.

I believe this is the ideal behaviour. The optimisation of
for(CONST..CONST) should be modified to match, providing read-only values.

>What I'd like:
>CONST..CONST to return something that you can modify.

Eww. We have enough ways already to create mutable values, and most
of them are a lot clearer than this about the scope of the variable.
If something looks constant then it probably should be.

-zefram


avarab at gmail

Nov 27, 2009, 2:36 PM

Post #17 of 18 (561 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Wed, Nov 25, 2009 at 00:52, Eric Brine <ikegami [at] adaelis> wrote:
> This makes 1..5 no different than 1,2,3,4,5 in terms of functionality, and
> avoids some very subtle bugs.

If this doesn't get accepted for 5.12 it would be very good to have a
version of your change t/op/range.t and other relevant tests that
passes with the current behavior in blead. Whatever the semantics end
up being they need to be tested.


ikegami at adaelis

Nov 27, 2009, 8:13 PM

Post #18 of 18 (554 views)
Permalink
Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only [In reply to]
On Fri, Nov 27, 2009 at 5:36 PM, Ævar Arnfjörð Bjarmason
<avarab [at] gmail>wrote:

> On Wed, Nov 25, 2009 at 00:52, Eric Brine <ikegami [at] adaelis> wrote:
> > This makes 1..5 no different than 1,2,3,4,5 in terms of functionality,
> and
> > avoids some very subtle bugs.
>
> If this doesn't get accepted for 5.12


It won't.


> it would be very good to have a
> version of your change t/op/range.t and other relevant tests that
> passes with the current behavior in blead. Whatever the semantics end
> up being they need to be tested.
>

Can do. Should it prove the bug like

is( ( join ' ', map { join '', map { ++$_ } 1..4 } 1..2 ), '2345 3456' );

or just should it simply be

is( ( join '', map { ++$_ } 1..4 ), '2345' );

Perl porters RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.