The Inbox: Collections-EG.908.mcz

A new version of Collections was added to project The Inbox:
http://source.squeak.org/inbox/Collections-EG.908.mcz

==================== Summary ====================

Name: Collections-EG.908
Author: EG
Time: 7 August 2020, 8:17:30.343164 pm
UUID: d02855a8-a339-4f1d-bede-89a50f07892e
Ancestors: Collections-eem.907

Changing  behavior of #peekBack to (correctly) return first element of underlying collection when stream position is 1.

=============== Diff against Collections-eem.907 ===============

Item was changed:
  ----- Method: PositionableStream>>peekBack (in category 'accessing') -----
  peekBack
  "Return the element at the previous position, without changing position.  Use indirect messages in case self is a StandardFileStream."
-
  | element |
  self position = 0 ifTrue: [self errorCantGoBack].
+ element := self back.
- self position = 1 ifTrue: [self position: 0.  ^ nil].
- self skip: -2.
- element := self next.
  self skip: 1.
  ^ element!

The new tests and fix for PositionableStream look good, but there is a
failure now in RWBinaryOrTextStreamTest>>testPeekBack.

Dave

On Sat, Aug 08, 2020 at 12:17:32AM +0000, [hidden email] wrote:

Actually, I think that the new ReadStreamTest>>testPeekBack is not in
agreement with the existing RWBinaryOrTextSteamTest>>testPeekBack.

The peekBack method was not in Squeak 3.6, but it appears in Squeak 3.8.
It is unused in the image, but it presumably is intended to support parsing
things from a stream.

If I understand correctly, peek means "show me the next element that
has not yet been read from the stream", and peekBack means "show me the
element before the one that was most recently read from the stream".
So I expect that peekBack would mean to skip back 2 from the current
position, not skip back 1.

Dave

On Fri, Aug 07, 2020 at 09:32:56PM -0400, David T. Lewis wrote:

Hi Dave,

On Fri, 7 Aug 2020, David T. Lewis wrote:

Another interpretation of peek in the context of streams is to look
around the current position without consuming any elements.
#peek returns what #next would return without consuming any elements,
and following the same logic, #peekBack is expected to return what #back
would without affecting position.

That interpretation is what Eric implemented.

If we decide to stick with the previous implementation based on #oldBack,
which does not make any practical sense to me, then we'll need another
method to get the last read element, because #peekBack would return the
penultimate one.


Levente

Hi Eric,

On Fri, 7 Aug 2020, Eric Gade wrote:
That sounds really cool. I hope it fixes the bug Karl reported early this
year[1] (see the attached gif if you don't know). When I had a look at the
code, I felt like rewriting it would be easier than fixing it.

Now that I know the context of where you intend to use #peekBack, I
suggest you shouldn't use it but rather store what #next returned in a
variable.
That should make a significant difference in performance if used in a
tight loop.


Levente

[1] http://forum.world.st/BUG-GIFReadWriter-td5109760.html

Hi Levente,

On Sat, Aug 08, 2020 at 06:32:56PM +0200, Levente Uzonyi wrote:
When I first looked at it, I had the same interpretation as Eric and you.

>
> If we decide to stick with the previous implementation based on #oldBack,
> which does not make any practical sense to me, then we'll need another
> method to get the last read element, because #peekBack would return the
> penultimate one.
>

You are right, it is inconsistent and confusing. But I wonder if there is
any use case for a method that just answers the most recently read item.

Sometimes I just look at Cuis when I want to find out how to do things right.
Here is what Cuis does:

  'ABCDEF' readStream next: 2; peekBack. ==> $B
  'ABCDEF' readStream next: 2; back. ==> $B
  'ABCDEF' readStream next: 2; oldPeekBack. ==> $A
 
  'ABCDEF' readStream next: 1; peekBack. ==> $A
  'ABCDEF' readStream next: 1; back. $A
  'ABCDEF' readStream next: 1; oldPeekBack. ==> nil
 
  'ABCDEF' readStream next: 0; peekBack. ==> Error: CantGoBack
  'ABCDEF' readStream next: 0; back. ==> Error: CantGoBack
  'ABCDEF' readStream next: 0; oldPeekBack. ==> Error: CantGoBack

In Cuis, there are no senders of #peekBack, and one sender of #oldPeekBack
(PositionableStream>>#backChunk).

I think this is consistent with what you are recommending, so maybe we
should just adopt the Cuis implmentations and update our tests accordingly.

Note that the #oldPeekBack in Cuis is like our current #peekBack, and is
still inconsistent with respect to whether it answers nil or raises an error,
which still should be fixed.

Dave