Here are a better set of patches for Rakudo. They don't duplicate the check
for $p < 0, and they avoid calling nqp::elems(), by assuming that nqp::atpos()
safely return nql::null() for indices beyond the end of the array.
(Which Parrot does. Is any of this spec'd anywhere?)
The NQP patches remain fine.
On Sun, Feb 10, 2013 at 09:19:22PM +0000, Nicholas Clark wrote:
> I've no idea if the coding style is appropriate. Whether the indentation is
> crazy. Whether one should be using Mu. etc
This still holds. Style-wise, it would be possible to avoid using return-rw
by using a nested ?? !! - which is considered better?
Nicholas Clark
>From b0c5f0c175bed831b8e89808b237d4484425b2ae Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Sun, 10 Feb 2013 18:44:59 +0100
Subject: [PATCH 1/4] Move the test for negative indices from postcircumfix:<[ ]> to at_pos().
at_pos() shouldn't accept negative indices when called directly either.
Having the check in two places duplicates work.
---
src/core/Any.pm | 13 +------------
src/core/Array.pm | 4 ++++
src/core/Buf.pm | 1 +
src/core/Capture.pm | 1 +
src/core/List.pm | 1 +
src/core/LoL.pm | 1 +
6 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/core/Any.pm b/src/core/Any.pm
index 2bca541..e6f6b58 100644
--- a/src/core/Any.pm
+++ b/src/core/Any.pm
@@ -153,7 +153,6 @@ my class Any {
X::Bind::ZenSlice.new(type => self.WHAT).throw
}
multi method postcircumfix:<[ ]>(\SELF: $pos) is rw {
- fail "Cannot use negative index $pos on {SELF.WHAT.perl}" if $pos < 0;
SELF.at_pos($pos)
}
multi method postcircumfix:<[ ]>($pos, Mu :$BIND! is parcel) is rw {
@@ -161,19 +160,15 @@ my class Any {
self.bind_pos($pos, $BIND)
}
multi method postcircumfix:<[ ]>($pos, :$p!) is rw {
- fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$p ?? RWPAIR($pos, self.at_pos($pos)) !! self.at_pos($pos)
}
multi method postcircumfix:<[ ]>($pos, :$kv!) is rw {
- fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$kv ?? ($pos, self.at_pos($pos)) !! self.at_pos($pos)
}
multi method postcircumfix:<[ ]>($pos, :$k!) is rw {
- fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$k ?? $pos !! self.at_pos($pos)
}
multi method postcircumfix:<[ ]>(\SELF: int $pos) is rw {
- fail "Cannot use negative index $pos on {SELF.WHAT.perl}" if $pos < 0;
SELF.at_pos($pos)
}
multi method postcircumfix:<[ ]>(int $pos, Mu :$BIND! is parcel) is rw {
@@ -181,20 +176,16 @@ my class Any {
self.bind_pos($pos, $BIND)
}
multi method postcircumfix:<[ ]>(int $pos, :$p!) is rw {
- fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$p ?? RWPAIR($pos, self.at_pos($pos)) !! self.at_pos($pos)
}
multi method postcircumfix:<[ ]>(int $pos, :$kv!) is rw {
- fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$kv ?? ($pos, self.at_pos($pos)) !! self.at_pos($pos)
}
multi method postcircumfix:<[ ]>(int $pos, :$k!) is rw {
- fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$k ?? $pos !! self.at_pos($pos)
}
multi method postcircumfix:<[ ]>(\SELF: Positional \pos) is rw {
if nqp::iscont(pos) {
- fail "Cannot use negative index {pos} on {SELF.WHAT.perl}" if pos < 0;
return SELF.at_pos(pos)
}
my $list = pos.flat;
@@ -205,7 +196,6 @@ my class Any {
}
multi method postcircumfix:<[ ]>(\SELF: Positional \pos, :$p!) is rw {
if nqp::iscont(pos) {
- fail "Cannot use negative index {pos} on {SELF.WHAT.perl}" if pos < 0;
return RWPAIR(pos, SELF.at_pos(pos))
}
my $list = pos.flat;
@@ -214,7 +204,6 @@ my class Any {
}
multi method postcircumfix:<[ ]>(\SELF: Positional \pos, :$kv!) is rw {
if nqp::iscont(pos) {
- fail "Cannot use negative index {pos} on {SELF.WHAT.perl}" if pos < 0;
return (pos, SELF.at_pos(pos))
}
my $list = pos.flat;
@@ -232,7 +221,6 @@ my class Any {
}
multi method postcircumfix:<[ ]>(\SELF: Positional \pos, :$v!) is rw {
if nqp::iscont(pos) {
- fail "Cannot use negative index {pos} on {SELF.WHAT.perl}" if pos < 0;
SELF.at_pos(pos)
}
my $list = pos.flat;
@@ -283,6 +271,7 @@ my class Any {
self;
}
multi method at_pos(Any:U \SELF: $pos) is rw {
+ fail "Cannot use negative index $pos on {SELF.WHAT.perl}" if $pos < 0;
pir::setattribute__0PPsP(my $v, Scalar, '$!whence',
-> { SELF.defined || &infix:<=>(SELF, Array.new);
SELF.bind_pos($pos, $v) });
diff --git a/src/core/Array.pm b/src/core/Array.pm
index cb67207..17083d3 100644
--- a/src/core/Array.pm
+++ b/src/core/Array.pm
@@ -13,6 +13,7 @@ class Array {
if nqp::isnanorinf($pos) {
X::Item.new(aggregate => self, index => $pos).throw;
}
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
my int $p = nqp::unbox_i($pos.Int);
my Mu $items := nqp::p6listitems(self);
# hotpath check for element existence (RT #111848)
@@ -24,6 +25,7 @@ class Array {
-> { nqp::bindpos($items, $p, $v) } )
}
multi method at_pos(Array:D: int $pos) is rw {
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
my Mu $items := nqp::p6listitems(self);
# hotpath check for element existence (RT #111848)
nqp::existspos($items, $pos)
@@ -106,6 +108,7 @@ class Array {
my role TypedArray[::TValue] does Positional[TValue] {
multi method at_pos($pos is copy, TValue $v? is copy) is rw {
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$pos = $pos.Int;
self.exists($pos)
?? nqp::atpos(nqp::getattr(self, List, '$!items'), nqp::unbox_i($pos))
@@ -113,6 +116,7 @@ class Array {
-> { nqp::bindpos(nqp::getattr(self, List, '$!items'), nqp::unbox_i($pos), $v) } )
}
multi method at_pos(int $pos, TValue $v? is copy) is rw {
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
self.exists($pos)
?? nqp::atpos(nqp::getattr(self, List, '$!items'), $pos)
!! pir::setattribute__0PPsP($v, Scalar, '$!whence',
diff --git a/src/core/Buf.pm b/src/core/Buf.pm
index 9560b26..7b58450 100644
--- a/src/core/Buf.pm
+++ b/src/core/Buf.pm
@@ -31,6 +31,7 @@ my class Buf does Positional {
}
method at_pos(Buf:D: Int:D $idx) {
+ fail "Cannot use negative index $idx on {self.WHAT.perl}" if $idx < 0;
nqp::p6box_i(nqp::ord(nqp::substr($!buffer, nqp::unbox_i($idx), 1)));
}
diff --git a/src/core/Capture.pm b/src/core/Capture.pm
index 20458c2..d21630b 100644
--- a/src/core/Capture.pm
+++ b/src/core/Capture.pm
@@ -17,6 +17,7 @@ my class Capture {
}
method at_pos(Capture:D: $pos is copy) {
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$pos = $pos.Int;
nqp::existspos($!list, nqp::unbox_i($pos))
?? nqp::atpos($!list, nqp::unbox_i($pos))
diff --git a/src/core/List.pm b/src/core/List.pm
index 84f7eff..c7428c5 100644
--- a/src/core/List.pm
+++ b/src/core/List.pm
@@ -73,6 +73,7 @@ my class List does Positional {
multi method at_pos(List:D: $pos is copy) is rw {
$pos = $pos.Int;
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
self.exists($pos)
?? nqp::atpos($!items, nqp::unbox_i($pos))
!! Nil
diff --git a/src/core/LoL.pm b/src/core/LoL.pm
index f57f613..3c2798d 100644
--- a/src/core/LoL.pm
+++ b/src/core/LoL.pm
@@ -9,6 +9,7 @@ class LoL {
}
method at_pos($pos is copy) {
+ fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$pos = $pos.Int;
self.exists($pos)
?? nqp::findmethod(List, 'at_pos')(self, $pos)
--
1.7.2.5
>From af14e5132c8829553cb1d2448655fbf11c14decc Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Sun, 10 Feb 2013 20:09:50 +0100
Subject: [PATCH 2/4] Refactor Array.at_pos() to remove the use of nqp::existspos().
---
src/core/Array.pm | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/core/Array.pm b/src/core/Array.pm
index 17083d3..0169b74 100644
--- a/src/core/Array.pm
+++ b/src/core/Array.pm
@@ -17,9 +17,10 @@ class Array {
my int $p = nqp::unbox_i($pos.Int);
my Mu $items := nqp::p6listitems(self);
# hotpath check for element existence (RT #111848)
- nqp::existspos($items, $p)
- || nqp::getattr(self, List, '$!nextiter').defined
- && self.exists($p)
+ my Mu $item := nqp::atpos($items, $p);
+ return-rw $item
+ if !nqp::isnull($item);
+ nqp::getattr(self, List, '$!nextiter').defined && self.exists($p)
?? nqp::atpos($items, $p)
!! pir::setattribute__0PPsP(my $v, Scalar, '$!whence',
-> { nqp::bindpos($items, $p, $v) } )
@@ -28,9 +29,10 @@ class Array {
fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
my Mu $items := nqp::p6listitems(self);
# hotpath check for element existence (RT #111848)
- nqp::existspos($items, $pos)
- || nqp::getattr(self, List, '$!nextiter').defined
- && self.exists($pos)
+ my Mu $item := nqp::atpos($items, $pos);
+ return-rw $item
+ if !nqp::isnull($item);
+ nqp::getattr(self, List, '$!nextiter').defined && self.exists($pos)
?? nqp::atpos($items, $pos)
!! pir::setattribute__0PPsP(my $v, Scalar, '$!whence',
-> { nqp::bindpos($items, $pos, $v) } )
--
1.7.2.5
>From 885d25afff4422f63f58823e9cdbf0455f64b206 Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Sun, 10 Feb 2013 20:30:22 +0100
Subject: [PATCH 3/4] Refactor List.exists() and .shift() to remove the use of nqp::existspos().
---
src/core/List.pm | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/core/List.pm b/src/core/List.pm
index c7428c5..d3b8691 100644
--- a/src/core/List.pm
+++ b/src/core/List.pm
@@ -95,7 +95,9 @@ my class List does Positional {
method exists(\pos) {
self.gimme(pos + 1);
- nqp::p6bool(nqp::existspos($!items, nqp::unbox_i(pos)))
+ my Mu $count := nqp::elems($!items);
+ return False if pos < 0 || pos > $count;
+ nqp::p6bool(!nqp::isnull(nqp::atpos($!items, nqp::unbox_i(pos))))
}
method gimme($n, :$sink) {
@@ -217,7 +219,7 @@ my class List does Positional {
method shift() is rw {
# make sure we have at least one item, then shift+return it
- nqp::islist($!items) && nqp::existspos($!items, 0) || self.gimme(1)
+ nqp::islist($!items) && !nqp::isnull(nqp::atpos($!items, 0)) || self.gimme(1)
?? nqp::shift($!items)
!! fail 'Element shifted from empty list';
}
--
1.7.2.5
>From 7d85d02d2bc9254dcfe36bc71c8b579eb103ed2e Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Sun, 10 Feb 2013 20:44:37 +0100
Subject: [PATCH 4/4] Refactor Capture::at_pos() to remove the use of nqp::existspos().
---
src/core/Capture.pm | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/core/Capture.pm b/src/core/Capture.pm
index d21630b..8933847 100644
--- a/src/core/Capture.pm
+++ b/src/core/Capture.pm
@@ -19,9 +19,10 @@ my class Capture {
method at_pos(Capture:D: $pos is copy) {
fail "Cannot use negative index $pos on {self.WHAT.perl}" if $pos < 0;
$pos = $pos.Int;
- nqp::existspos($!list, nqp::unbox_i($pos))
- ?? nqp::atpos($!list, nqp::unbox_i($pos))
- !! Any
+ my Mu $item := nqp::atpos($!list, nqp::unbox_i($pos));
+ nqp::isnull($item)
+ ?? Any
+ !! $item
}
method hash(Capture:D:) {
--
1.7.2.5
