2014-09-03 14:44 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:

I think we should change

OrderedCollection>>#do: aBlock
�� "Override the superclass for performance reasons."

�� firstIndex to: lastIndex do: [ :index |
�� aBlock value: (array at: index) ]

I agree. It's easier to read. But if you do that world menu does not work any more :-/

��

Clement,

Do you happen to know exactly where the world menu building goes wrong ?

I think so.

The problem is in methods such as

menuCommandOn: aBuilder��

<worldMenu>��

(aBuilder group: #SystemChanges)

parent: #Tools; ��

order: 0.51;

with: [

(aBuilder item: #'Change Sorter')

action:[self open];��

icon: self taskbarIcon.

(aBuilder item: #'Recover lost changes...')��

icon: Smalltalk ui icons recoverLostChangesIcon;

action: [Smalltalk tools changeList browseRecentLog].].

aBuilder withSeparatorAfter.

Here you have a with block which add other items to the menu.

Then in the method PragmaMenuBuilder>>#interpretRegistration:, while iterating other the registrations,

The code "node with: block" line 12 calls PragmaMenuBuilder>>#currentRoot:while: which activates the block, adding new elements to the collection it iterates from in PragmaMenuBuilder>>#interpretRegistration:.

Be ready to use haltOnce / inspectOnce to debug these cases :-).

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
>>
>