[squeak-dev] The Inbox: Collections-cmm.576.mcz

Frank Shearar frank.shearar at gmail.com
Thu Jul 24 15:50:29 UTC 2014


On 24 July 2014 16:39,  <commits at source.squeak.org> wrote:
> Chris Muller uploaded a new version of Collections to project The Inbox:
> http://source.squeak.org/inbox/Collections-cmm.576.mcz
>
> ==================== Summary ====================
>
> Name: Collections-cmm.576
> Author: cmm
> Time: 24 July 2014, 10:39:06.452 am
> UUID: 17b60ac6-55b3-4706-b1c8-d826e6ad5657
> Ancestors: Collections-eem.575
>
> When a SequenceableCollection contains duplicate keys, #findBinaryIndex:do:ifNone: should answer the index of the _earliest_.
>
> =============== Diff against Collections-eem.575 ===============
>
> Item was changed:
>   ----- Method: SequenceableCollection>>findBinaryIndex:do:ifNone: (in category 'enumerating') -----
> + findBinaryIndex: aBlock do: actionBlock ifNone: exceptionBlock
> - findBinaryIndex: aBlock do: actionBlock ifNone: exceptionBlock
>         "Search for an element in the receiver using binary search.
>         The argument aBlock is a one-element block returning
>                 0       - if the element is the one searched for
>                 <0      - if the search should continue in the first half
>                 >0      - if the search should continue in the second half
>         If found, evaluate actionBlock with the index as argument
>         If no matching element is found, evaluate exceptionBlock,
>         with the indexes of the 'bounding' elements as optional
>         arguments.      Warning: Might give invalid indexes, see
>         examples below.
>         Examples:
> +               #(1 3 5 7 11 11 15 23)
> -               #(1 3 5 7 11 15 23)
>                         findBinaryIndex: [ :arg | 11 - arg ]
>                         do: [ :found | found ]
>                         ifNone: [ :a :b | ('between: ', {a. b} printString)]
>                 #(1 3 5 7 11 15 23)
>                         findBinaryIndex: [ :arg | 12 - arg ]
>                         do: [ :found | found ]
>                         ifNone: [ :a :b | ('between: ', {a. b} printString) ]
>                 #(1 3 5 7 11 15 23) d
>                         findBinaryIndex: [ :arg | 0.5 - arg ]
>                         do: [ :found | found ]
>                         ifNone: [ :a :b | ('between: ', {a. b} printString) ]
>                 #(1 3 5 7 11 15 23)
>                         findBinaryIndex: [ :arg | 25 - arg ]
>                         do: [ :found | found ]
>                         ifNone: [ :a :b | ('between: ',{a. b} printString) ]
>         "
>         | index low high |
>         low := 1.
>         high := self size.
> +       [ index := high + low // 2.
> +       low > high ] whileFalse:
> +               [ | test |
> +               test := aBlock value: (self at: index).
> +               test = 0
> +                       ifTrue:
> +                               [ "In case there are duplicate keys, walk backward to the first."
> +                               [ index > 1 and: [ (aBlock value: (self at: index - 1)) = 0 ] ] whileTrue: [ index := index - 1 ].
> +                               ^ actionBlock value: index ]
> +                       ifFalse:
> +                               [ test > 0
> -       [
> -               index := high + low // 2.
> -               low > high ] whileFalse: [
> -                       | test |
> -                       test := aBlock value: (self at: index).
> -                       test = 0
> -                               ifTrue: [ ^actionBlock value: index ]
> -                               ifFalse: [ test > 0
>                                         ifTrue: [ low := index + 1 ]
>                                         ifFalse: [ high := index - 1 ] ] ].
> +       ^ exceptionBlock
> +               cull: high
> +               cull: low!
> -       ^exceptionBlock cull: high cull: low!

The change looks good, and certainly I approve of the feature.
Wouldn't mind seeing some tests to go with it too, especially if you
took the time to backfill the other tests this method needs!

(It took me a good few minutes to parse the diff and see that the
_actual_ change is just (something like)

> +                               [ "In case there are duplicate keys, walk backward to the first."
> +                               [ index > 1 and: [ (aBlock value: (self at: index - 1)) = 0 ] ] whileTrue: [ index := index - 1 ].
> -                               ifTrue: [ ^actionBlock value: index ]


It's _really hard_ to see what the actual change is, when it's drowned
out by semantically meaningless whitespace changes. (If you were in an
image and did a "pretty print diff" you'd get an accurate diff, at the
expense of not seeing the horrible things the author was doing to the
layout (regardless of what layout you, the viewer, preferred).))

frank


More information about the Squeak-dev mailing list