I think we should change
OrderedCollection>>#do: aBlock
"Override the superclass for performance reasons."
firstIndex to: lastIndex do: [ :index |
aBlock value: (array at: index) ]
Clement,
Do you happen to know exactly where the world menu building goes wrong ?
Sven
On 03 Sep 2014, at 13:51, Sven Van Caekenberghe <[email protected]> wrote:
> Clement,
>
> I am not sure, but could this not be a compiler issue ?
> Not with Opal but with the old compiler ?!
>
> I mean, if you look at the implementations of OrderedCollection>>#do: and
> OrderedCollection>>#collect: they basically do the same thing, going from
> firstIndex to lastIndex, both of which are instance variables, but in the
> first case, the #do:, the end test is evaluated on each iteration, while in
> the second case, the #to:do:, the end test should be evaluated only once.
> Since lastIndex changes while iterating, one iteration sees it, the other not.
>
> Looking at the byte codes seems to confirm that: the compilation of the
> #:to:do is different: Opal correctly moves both instance variables to temps,
> while the old compiler keeps on referring to the second instance variable
> (lastIndex) directly on each loop - which is plain wrong IMHO.
>
> I am with Marcus that you should not modify a collection that you iterate
> over. What if you would be inserting values at arbitrary places ?
>
> I am with you that both (all) enumerations should be consistent. Like in VW,
> delegating to the simplest #do: seems best.
>
> Sven
>
> On 03 Sep 2014, at 11:42, Clément Bera <[email protected]> wrote:
>
>> Hello guys,
>>
>> I was looking into the OrderedCollection protocols recently to see how well
>> the sista optimizer perform with it methods, and I realized that this is
>> completely broken.
>>
>> For example:
>>
>> col := #(1 2 3 4 5) asOrderedCollection.
>> col do: [ :elem | elem trace .
>> elem < 4 ifTrue: [ col add: col size + 1 ]].
>>
>> => '12345678'
>>
>> However:
>>
>> col := #(1 2 3 4 5) asOrderedCollection.
>> col collect: [ :elem | elem trace .
>> elem < 4 ifTrue: [ col add: col size + 1 ]].
>>
>> => '12345'
>>
>> This means that #do: and #reverseDo: iterate over all the elements of the
>> collection,*including* the ones that you are adding while iterating over the
>> collection, whereas all the other OrderedCollection protocols, such as
>> #collect:, #select:, iterates over all the elements of the collection,
>> *excluding* the ones you are adding while iterating over the collection.
>>
>> Marcus argued that one should not edit a collection while iterating over it,
>> however this point is not valid as the World menu relies on this feature,
>> using #do: to iterate over the elements of the OrderedCollection including
>> the one it is adding while iterating over the collection. Changing the
>> implementation makes the world menu display half of its items.
>>
>> I don't like this difference because it is inconsistent. For example,
>> refactoring a #do: into a #collect: can simply not work because they do not
>> iterate over the same elements if you are editing the collection while
>> iterating over it.
>>
>> In VW, the protocols are consistent and iterating over a collection never
>> iterates over the elements one is adding while iterating over it. Therefore,
>> I believe most frameworks should expect this behavior (at least the ones
>> cross smalltalk) which sounds the most correct.
>>
>> I think we should fix the world menu implementation and make the protocols
>> consistent. Alternatively, we can let VW be a much more consistent Smalltalk
>> environment than Pharo. What do you think ?
>>
>> Clement
>>
>
