Stream nextPutAll: with an OrderedCollection go boom

I spotted a previous mention of this in the old maillist archives but it is still a current failure -

        (OrderedCollection
                streamContents:
                        [:strm |
                                strm
                                        nextPutAll: (OrderedCollection with: 1 ) ])

Boom!

It's fine if we add asArray

I'm suspicious of WriteStream>>#nextPutAll: making the assumption that SequenceableCollection>>#replaceFrom:to:with:startingAt: is always ok. Clearly not for OrderedCollection...

Looks to me that we need to make OrderedCollection not pass the check for the 'fast path'? How about changing the #isBits test for a more capability oriented #canAcceptFastPathNextPut ?

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
A bug in the code is worth two in the documentation.

Hi Tim,

On Tue, 30 Mar 2021, tim Rowledge wrote:

> I spotted a previous mention of this in the old maillist archives but it is still a current failure -
>
> (OrderedCollection
> streamContents:
> [:strm |
> strm
> nextPutAll: (OrderedCollection with: 1 ) ])
>
> Boom!

I still agree with my previous answers[1]: don't do that.
Just use OrderedCollection's own API if you want to build a collection.

>
> It's fine if we add asArray
>
> I'm suspicious of WriteStream>>#nextPutAll: making the assumption that SequenceableCollection>>#replaceFrom:to:with:startingAt: is always ok. Clearly not for OrderedCollection...
>
> Looks to me that we need to make OrderedCollection not pass the check for the 'fast path'? How about changing the #isBits test for a more capability oriented #canAcceptFastPathNextPut ?

There's no need to change anything there to support a bad pattern. If you
really-really want to make it work, just override #new:streamContents::

OrderedCollection class >> new: newSize streamContents: blockWithArg

  | contents |
  contents := self arrayType new: newSize streamContents: blockWithArg.
  ^self basicNew
  setContents: contents;
  yourself


Levente

[1] http://forum.world.st/streamContents-on-OrderedCollection-broken-td4760369.html

>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> A bug in the code is worth two in the documentation.

> On 2021-03-30, at 1:05 PM, Levente Uzonyi <[hidden email]> wrote:
>
> I still agree with my previous answers[1]: don't do that. Just use OrderedCollection's own API if you want to build a collection.

Well, yeah, but.

I like things to be decently uniform and at least try to meet the implicit contract. In this case the problem is purely because the test to do a 'fast hack' wrongly passes.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Security announcement - as of next week, passwords will be entered in Morse code.

On Tue, Mar 30, 2021 at 02:31:41PM -0700, tim Rowledge wrote:
Trying to follow the discussion. Where is the "test to do a 'fast hack'" that wrongly passes?

Dave

On Tue, 30 Mar 2021, tim Rowledge wrote:

>
>
>> On 2021-03-30, at 1:05 PM, Levente Uzonyi <[hidden email]> wrote:
>>
>> I still agree with my previous answers[1]: don't do that. Just use OrderedCollection's own API if you want to build a collection.
>
> Well, yeah, but.
>
> I like things to be decently uniform and at least try to meet the implicit contract. In this case the problem is purely because the test to do a 'fast hack' wrongly passes.

No, that's not the problem. The problem is that OrderedCollection >> #new:
returns a collection that is always empty.

When WriteStream >> #nextPutAll: sees the empty collection, it tries to
make enough room for the elements to be added by sending #growTo: to
itself.
#growTo: sends #new: to the collection's class, which in case of
OrderedCollection will return another empty collection.
So, collection will remain empty after #growTo:.
#nextPutAll: rightfully assumes that now there is enough space for the
elements to be added, but there's still none, hence the error.

The real solution is to change WriteStream >> #growTo: to

WriteStream >> growTo: anInteger
  " anInteger is the required minimal new size of the collection "

  | oldSize newSize |
  oldSize := collection size.
  newSize := anInteger + (oldSize // 4 max: 20).
  collection := collection grownBy: newSize - oldSize.
  writeLimit := collection size.

It has 5 senders in the WriteStream hierarchy in my image. All of those
unnecessarily add 10 to the requested new size, even through #growTo: will
add at least 20 extra slots.

Collections-ul.932 fixes that.
There's also Collections-ul.933 in the Inbox which slightly improves the
performance of OrderedCollection streamContents: by avoiding allocating
an empty initial collection. If you find that useful, push it to the
Trunk.


But even with that fix, there will be several subclasses of
SequenceableCollection which won't work with WriteStreams and
#streamContents:.


Levente

>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> Security announcement - as of next week, passwords will be entered in Morse code.