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 <sven@stfx.eu> 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 <bera.clement@gmail.com> 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