Hi Levente,
thanks for the suggestions.
And one more thing: These method resemble #indexOf:* methods, so I think #ifAbsent: is a better choice than #ifNone:.
Agreed :-)
I suggest keeping the all method comments
Personally, I see little value in documenting a one-liner. Documentation is useful for explaining and optimized method that does not provide the typical Smalltalk readability. Plus, they all need to be maintained ...
measuring the performance impact of the rewrite.
What is the critical point here? Of course, the stack will be a little higher, but the asymptotic cost does not change. We only evaluate anotherBlock at the end instead of returning 0.
Please do not misunderstand me: I honor optimizing critical Smalltalk methods, but I always strive to avoid premature optimization. Should we maybe establish to practice to store such benchmarks somewhere in the trunk so that not every willing contributor needs to write them again?
Best, Christoph
________________________________ Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Levente Uzonyi leves@caesar.elte.hu Gesendet: Sonntag, 16. Februar 2020 22:47:10 An: The general-purpose Squeak developers list Betreff: Re: [squeak-dev] The Inbox: Collections-ct.875.mcz
And one more thing: These method resemble #indexOf:* methods, so I think #ifAbsent: is a better choice than #ifNone:.
Levente
On Sun, 16 Feb 2020, Levente Uzonyi wrote:
Hi Christoph,
I suggest keeping the all method comments, and measuring the performance impact of the rewrite.
Levente
On Sun, 16 Feb 2020, commits@source.squeak.org wrote:
A new version of Collections was added to project The Inbox: http://source.squeak.org/inbox/Collections-ct.875.mcz
==================== Summary ====================
Name: Collections-ct.875 Author: ct Time: 16 February 2020, 3:32:23.116 pm UUID: 61ea773b-1408-c040-a712-09238ee6fe2f Ancestors: Collections-topa.873
Extends and realigns version of #findFirst: and #findLast:.
=============== Diff against Collections-topa.873 ===============
Item was changed: ----- Method: SequenceableCollection>>findFirst: (in category
'enumerating') -----
findFirst: aBlock
"Return the index of my first element for which aBlock evaluates astrue."
^ self findFirst: aBlock startingAt: 1!
| index |index := 0.[(index := index + 1) <= self size] whileTrue:[(aBlock value: (self at: index)) ifTrue: [^index]].^ 0!Item was added:
- ----- Method: SequenceableCollection>>findFirst:ifNone: (in category
'enumerating') -----
- findFirst: aBlock ifNone: anotherBlock
^ self findFirst: aBlock startingAt: 1 ifNone: anotherBlock!Item was added:
- ----- Method: SequenceableCollection>>findFirst:startingAt: (in category
'enumerating') -----
- findFirst: aBlock startingAt: minIndex
^ self findFirst: aBlock startingAt: minIndex ifNone: [0]!Item was added:
- ----- Method: SequenceableCollection>>findFirst:startingAt:ifNone: (in
category 'enumerating') -----
- findFirst: aBlock startingAt: minIndex ifNone: anotherBlock
"Return the index of my first element with index >= minIndex forwhich aBlock evaluates as true. If no element is found, return the value of anotherBlock."
| index |index := minIndex - 1.[(index := index + 1) <= self size] whileTrue:[(aBlock value: (self at: index)) ifTrue: [^index]].^ anotherBlock value!Item was changed: ----- Method: SequenceableCollection>>findLast: (in category
'enumerating') -----
findLast: aBlock
"Return the index of my last element for which aBlock evaluates astrue."
^ self findLast: aBlock startingAt: 1!
| index |index := self size + 1.[(index := index - 1) >= 1] whileTrue:[(aBlock value: (self at: index)) ifTrue: [^index]].^ 0!Item was added:
- ----- Method: SequenceableCollection>>findLast:ifNone: (in category
'enumerating') -----
- findLast: aBlock ifNone: anotherBlock
^ self findLast: aBlock startingAt: 1 ifNone: anotherBlock!Item was changed: ----- Method: SequenceableCollection>>findLast:startingAt: (in category
'enumerating') -----
- findLast: aBlock startingAt: minIndex
- findLast: aBlock startingAt: i
"Return the index of my last element with index >= i for which aBlockevaluates as true."
^ self findLast: aBlock startingAt: minIndex ifNone: [0]!
| index |index := self size + 1.[(index := index - 1) >= i] whileTrue:[(aBlock value: (self at: index)) ifTrue: [^index]].^ 0!Item was added:
- ----- Method: SequenceableCollection>>findLast:startingAt:ifNone: (in
category 'enumerating') -----
- findLast: aBlock startingAt: minIndex ifNone: anotherBlock
"Return the index of my last element with index >= minIndex for whichaBlock evaluates as true. If no element is found, return the value of anotherBlock."
| index |index := self size + 1.[(index := index - 1) >= minIndex] whileTrue:[(aBlock value: (self at: index)) ifTrue: [^index]].^ anotherBlock value!