On Mon, Feb 11, 2013 at 08:11:53AM -0600, Patrick R. Michaud wrote:
> On Mon, Feb 11, 2013 at 09:56:31AM +0000, Nicholas Clark wrote:
> > 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?)
>
> AFAIK, Parrot has never spec'd that elements beyond the end of array
> return PMCNULL, which is one reason I've avoided relying on it.
> Also, I'm concerned that other VM platforms might not have the same
> characteristic as Parrot, while I am sure that nqp::elems() will
> be available and will be inexpensive.
OK. Currently, Jonathan's prototype code for Java ends up calling this
public SixModelObject at_pos_boxed(ThreadContext tc, long index) {
if (index < 0) {
index += elems;
if (index < 0)
throw new RuntimeException("VMArray: Index out of bounds");
}
else if (index >= elems)
return null;
return slots[start + (int)index];
}
with this caveat above it:
/**
* This is a fairly direct port of the QRPA logic implemented by Patrick
Michaud in
* the NQP repository. Thus the C-ish nature of the code. :-)
*/
I guess all this means that we're trying to get some idea of what the edge
case expectations are, and should be, for the NQP ops. And hence how
defensive the "platform" code such as the above needs to be. So, in this case,
if nqp::atpos is only required to accept indices between 0 and elems-1
inclusive, then platform code (such as the above) can become simpler, but
the NQP (and Rakudo) code has to be a tiny bit longer.
But, without any of this nailed down, it's worse, as the above code *feels*
necessary, but things like the negative index handling and overlarge index
handling are never actually used.
> > This still holds. Style-wise, it would be possible to avoid using return-rw
> > by using a nested ?? !! - which is considered better?
>
> Since return-rw involves extra function calls (to return-rw, for one,
> plus the internal code blocks it uses) and various exception handlers
> to capture and handle the return, it's definitely worth avoiding
> when not needed.
Following your above comments I've refactored the Array code like this:
multi method at_pos(Array:D: $pos) is rw {
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)
$p < nqp::elems($items)
&& !nqp::isnull(my Mu $item := nqp::atpos($items, $p))
?? $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) } )
}
which avoids return-rw, and avoids assuming behaviour for nqp::atpos for
beyond the end.
(Yes, this only makes sense if the goal is to remove eixstspos and deletepos
from NQP)
Nicholas Clark
>From e39b8cc3a75fa27175e9ba3abcf2e957e0165652 Mon Sep 17 00:00:00 2001
From: Nicholas Clark <n...@ccl4.org>
Date: Sun, 10 Feb 2013 20:09:50 +0100
Subject: [PATCH 1/3] Refactor Array.at_pos() to remove the use of nqp::existspos().
---
src/core/Array.pm | 26 ++++++++++++++------------
1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/src/core/Array.pm b/src/core/Array.pm
index 17083d3..b9d5f34 100644
--- a/src/core/Array.pm
+++ b/src/core/Array.pm
@@ -17,23 +17,25 @@ 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)
- ?? nqp::atpos($items, $p)
- !! pir::setattribute__0PPsP(my $v, Scalar, '$!whence',
- -> { nqp::bindpos($items, $p, $v) } )
+ $p < nqp::elems($items)
+ && !nqp::isnull(my Mu $item := nqp::atpos($items, $p))
+ ?? $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) } )
}
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)
- || 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) } )
+ $pos < nqp::elems($items)
+ && !nqp::isnull(my Mu $item := nqp::atpos($items, $pos))
+ ?? $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) } )
}
proto method bind_pos(|) { * }
--
1.7.2.5
