Thanks for the explanation, Nicolas! This sounds reasonable. This kind of conclusion was missing for me when I read through the different bug reports on Mantis. Adrian On Oct 9, 2008, at 22:18 , nicolas cellier wrote:
http://bugs.squeak.org/view.php?id=7180 +6455 +1603 +1602
Patches for Interval indexOf: and includes:
I wonder about these fixes because of http://code.google.com/p/pharo/issues/detail?id=35
Did you reconsider the correctness of these fixes? They seem complicated and after reading through the bug reports again (especially http://bugs.squeak.org/view.php?id=7180) I concluded that they may introduce more problems than they fix. I know I tried to find my way there too. Now I trusted nicolas. Nicolas are you strong on these fixes?
Stef
Adrian _______________________________________________ Pharo-project mailing list Pharo-project at lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Thank you Stef. Yes, the fix is safe. I develop here after these 3 points A)- The code was wrong B)- Discussions were too long, but the fixes are very easy C)- library could be enhanced
Adrian, hope I will convince you (also, read the code).
Cheers
Nicolas
A) The code was wrong
Prior this patch: - #includes: implementation were WRONG. - #indexOf: was missing, falling back to naive super slow implementation
Try these two tests in any Squeak image of your choice (3.7 to: 3.10):
"There is no reason that the first is true while the second is false" {(Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 + 1.0e-11. (Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 - 1.0e-11}.
"There is no reason for this timeToRun to grow" (1 to: 7) collect: [:i | [(1 to: 10000000) indexOf: (10 raisedTo: i)] timeToRun]
B)- the fixes are very easy
I simply implemented indexOf: with same algorithm as #includes: (corrected!). Then removed includes: because super includes: is implemented sending indexOf: Then I removed valueIncludes: because includes: was sole sender. You see, two methods removed, one fix added: no revolution
C)- library could be enhanced
What I would prefer is: - implement a method with an argument so that user can choose comparison tolerance - expand this fuzzy inclusion test to any other collection Maybe something like: includes: aNumber moreOrLess: aTolerance. Then Interval includes: could eventually use a default tolerance. But the obvious bugs must first be corrected...
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project