Buggy ExternalArrays in D5.1 pl2

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Buggy ExternalArrays in D5.1 pl2

Chris Uppal-3
Hi,

I've just updated to pl2.  I've found that neither WORDArray nor SWORDArray
implement the required #uncheckedAt:[put:] methods.

Also the new version of ExternalArray>>from:to:keysAndValuesDo: checks both
bounds before entering the loop.   That is inconsistant with all the other
implementations of that method (that I've checked) which do not throw a
BoundsError until after itterating over whichever elements are legal.  E.g:

    #( 1 2 3 4 )
            from: 2
            to: 7
            keysAndValuesDo: [:k :v | Transcript display: k; cr].

will write 2 3 and 4 to the transcript before throwing.

I can see that its been done that way for efficiency, but I suggest that an
implementation like (untested):

------------------------------
from: startInteger to: stopInteger keysAndValuesDo: operation
    "Evaluate the <dyadicValuable>, operation, for each element of the receiver
    between the <integer> indices, start and stop, inclusive with the element
and its
    index as respectively the second and first arguments."

    | stop |

    startInteger > stopInteger ifTrue: [^ self].
    startInteger < 1 ifTrue: [self errorSubscriptBounds: startInteger].
    startInteger > self size ifTrue: [self errorSubscriptBounds: stopInteger].

    "slightly convoluted expression to avoid bounds tests on every iteration"
    stop := self size min: stopInteger.
     self
            uncheckedFrom: startInteger
            to: stop
            keysAndValuesDo: operation.
    stop < stopInteger ifTrue: [self errorSubscriptBounds: stop + 1].
------------------------------

would be better.

    -- chris


Reply | Threaded
Open this post in threaded view
|

Re: Buggy ExternalArrays in D5.1 pl2

Blair McGlashan
"Chris Uppal" <[hidden email]> wrote in message
news:3fb4ec3a$0$107$[hidden email]...
> Hi,
>
> I've just updated to pl2.  I've found that neither WORDArray nor
SWORDArray
> implement the required #uncheckedAt:[put:] methods.

Thanks, we'll fix that in PL3, a small patch that we'll be releasing soon,
probably next week.

>
> Also the new version of ExternalArray>>from:to:keysAndValuesDo: checks
both
> bounds before entering the loop.   That is inconsistant with all the other
> implementations of that method (that I've checked) which do not throw a
> BoundsError until after itterating over whichever elements are legal.
> ...

Either definition would be compatible with the ANSI definition of the
<sequencedReadableCollection> message.
If anything I would say the second case is an implementation artefact and
arguably less correct, since the whole message is erroneous. Regardless I
don't think one should rely on when the error gets raised if that is not
explicitly specified.

Regards

Blair


Reply | Threaded
Open this post in threaded view
|

Re: Buggy ExternalArrays in D5.1 pl2

Chris Uppal-3
Blair McGlashan wrote:

> > Also the new version of ExternalArray>>from:to:keysAndValuesDo: checks
> > both bounds before entering the loop.   That is inconsistant with all
> > the other implementations of that method (that I've checked) which do
> > not throw a BoundsError until after itterating over whichever elements
> > are legal. ...
>
> Either definition would be compatible with the ANSI definition of the
> <sequencedReadableCollection> message.
> If anything I would say the second case is an implementation artefact and
> arguably less correct, since the whole message is erroneous. Regardless I
> don't think one should rely on when the error gets raised if that is not
> explicitly specified.

It's no big deal, but it seems to me that consistancy is worthwhile even if the
behaviour isn't conforming to a (semi-)formal specification -- the fact that
the other implementations (of that message in Dolphin -- not all the other
implementations of that message in other Smalltalks) behave in a certain way
suggests that that is the way it is *supposed* to behave.

But, as I said, it's no big deal, and I already had the code in place to
prevent the inconsistancy from "leaking" out from ExternalArray into my own
objects' implementations of #from:to:keysAndValuesDo:.

    -- chris


Reply | Threaded
Open this post in threaded view
|

Re: Buggy ExternalArrays in D5.1 pl2

Blair McGlashan-2
"Chris Uppal" <[hidden email]> wrote in message
news:3fb8e826$1$122$[hidden email]...

> Blair McGlashan wrote:
>
> > > Also the new version of ExternalArray>>from:to:keysAndValuesDo: checks
> > > both bounds before entering the loop.   That is inconsistant with all
> > > the other implementations of that method (that I've checked) which do
> > > not throw a BoundsError until after itterating over whichever elements
> > > are legal. ...
> >
> > Either definition would be compatible with the ANSI definition of the
> > <sequencedReadableCollection> message.
> > If anything I would say the second case is an implementation artefact
and
> > arguably less correct, since the whole message is erroneous. Regardless
I
> > don't think one should rely on when the error gets raised if that is not
> > explicitly specified.
>
> It's no big deal, but it seems to me that consistancy is worthwhile even
if the
> behaviour isn't conforming to a (semi-)formal specification -- the fact
that
> the other implementations (of that message in Dolphin -- not all the other
> implementations of that message in other Smalltalks) behave in a certain
way
> suggests that that is the way it is *supposed* to behave.
>

I don't think I had any such intent when I wrote them. Giving me the benefit
of the doubt it might have been because I knew the error would be detected
eventually by the #at: check, and I didn't want to duplicate that with
another bounds test. On the other hand it might just have been laziness.

Let's consider those other implementations anyway. In the base system
(ignoring database and COM stuff) there are 4 implementations:
    SequenceableCollection
    OrderedCollection
    Interval
    ExternalArray

Of these, the SequenceableCollection implementation may enumerate entries in
the collection before detecting the bounds error. I consider this a "lazy"
implementation (you may not agree). OrderedCollection's implementation fails
to detect out of bounds errors in certain cases where the elements do not
start and stop at the physical bounds of the collection (i.e. the firstIndex
> 1 and/or lastIndex < collection basicSize). Interval's implementation
fails to detect bounds errors at all. Now the OC and Interval
implementations are optimised forms; I think these errors arose because the
bounds checks needed by the message are not clearly expressed in the
SequenceableCollection implementation, so when it was copied down and
optimised the need to then implement bounds checks was overlooked.

Its been useful to consider this in detail since doing so lead me to write
some tests, at which point I discovered errors in two of the four
implementations. I intend to fix these by performing the bounds check up
front as in the ExternalArray implementation, which I now consider to be the
canonical form.

Thanks for bringing it up.

Regards

Blair


Reply | Threaded
Open this post in threaded view
|

Re: Buggy ExternalArrays in D5.1 pl2

Chris Uppal-3
Blair McGlashan wrote:

> Its been useful to consider this in detail since doing so lead me to write
> some tests, at which point I discovered errors in two of the four
> implementations. I intend to fix these by performing the bounds check up
> front as in the ExternalArray implementation, which I now consider to be
> the canonical form.

Does that mean that SequencableCollection will (be scheduled to) change too ?

I don't really mind either way, as long as I know what I'm supposed to be
consistant *with*.

    -- chris


Reply | Threaded
Open this post in threaded view
|

Re: Buggy ExternalArrays in D5.1 pl2

Blair McGlashan-2
"Chris Uppal" <[hidden email]> wrote in message
news:3fba3c87$0$113$[hidden email]...
> Blair McGlashan wrote:
>
> > Its been useful to consider this in detail since doing so lead me to
write
> > some tests, at which point I discovered errors in two of the four
> > implementations. I intend to fix these by performing the bounds check up
> > front as in the ExternalArray implementation, which I now consider to be
> > the canonical form.
>
> Does that mean that SequencableCollection will (be scheduled to) change
too ?

Yes. The cleanest way to fix the bugs is to rename the existing
implementations of #from:to:keysAndValuesDo: to
#uncheckedFrom:to:keysAndValuesDo: and copy the implementation of
#from:to:keysAndValuesDo: from ExternalArray into SequenceableCollection.

Regards

Blair