2014-09-03 14:44 GMT+02:00 Sven Van Caekenberghe <[email protected]>: > 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 <[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 > >> > > > > >
