Status: Accepted
Owner: marianopeck Labels: Type-Bug New issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations: testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each new] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot} So, my I think we should change #= in those classes by adding: self == anObject ifTrue: [ ^ true ]. self species = anObject species ifFalse: [ ^ false ]. at the beginning of the #= _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Updates:
Status: FixReviewNeeded Cc: [hidden email] [hidden email] Labels: Milestone-2.0 Comment #1 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 Fix in inbox Name: SLICE-Issue-5997-Classes-that-implement-unsafe-equals-MarianoMartinezPeck.1 Author: MarianoMartinezPeck Time: 30 May 2012, 5:20:23.426 pm UUID: 0b662b55-51da-4910-8e2c-b6406b7e7568 Ancestors: Dependencies: Monticello-MarianoMartinezPeck.602, Keymapping-Core-MarianoMartinezPeck.125 fix to issue 5997 _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #2 on issue 5997 by [hidden email]: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 Is the early exit if == really worth the extra code? As for the results list, granted the above is a yes, KMKeymap. MCMockDefinition. MCSnapshot seem fixed, Magnitude and WideCharacterSet didn't need any fixing, but ScaledDecimal still does a class = class test instead of species = species as far as I can tell from a merge view. Cheers, Henry _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #3 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 "Is the early exit if == really worth the extra code?" -> I didn't understand. "As for the results list, granted the above is a yes, KMKeymap. MCMockDefinition. MCSnapshot seem fixed, Magnitude and WideCharacterSet didn't need any fixing" -> exactyly. You are right about ScaledDecimal, it was already there, and yes, it was doing class = class. I didn't change it because "it was there" hehehe. But if you think it should be changed anyway, I can do it. _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #4 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 let's say it in another way: there are much more impl of #= that send #class instead of #species and I didnt fix them. should we? _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #5 on issue 5997 by [hidden email]: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 Hehe, I like consistency, I like species over class in = for the added flexibility, so if all my involvement would be answering "yes" to that question, I'd say do it :) _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Updates:
Status: Workneeded Comment #6 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 hehehehe ok. I need to write my thesis now. So I will give it a try tonight or so. _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #7 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 Sorry, but it is too much: (((SystemNavigation default allSendersOf: #class) collect: [:each | each compiledMethod ]) select: [:each | each selector = #=]) size -> 58 _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Updates:
Status: FixToInclude Comment #8 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 so, let's be agile and integrate first this version. If someone wants to change #class by #species in the remaining 58 methods, then we open another issue for that. _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #9 on issue 5997 by [hidden email]: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 I think it's wrong, species is not specifically dedicated to =, and not all = should use species. I think species was originally for select: collect: For example, I would like (1 to: 10000000) ~= (1 to: 10000000) asArray, as you can see their hash are different, and I don't think that applying Array hash to such interval would be very efficient. Still, I would like to have (1 to: 10) collect: use an Array species. _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Comment #10 on issue 5997 by marianopeck: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 esteban did you integrated? therefore we have to mark it as integrated _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Updates:
Status: Integrated Comment #11 on issue 5997 by [hidden email]: Classes that implement unsafe equals http://code.google.com/p/pharo/issues/detail?id=5997 in 2.0 110 _______________________________________________ Pharo-bugtracker mailing list [hidden email] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker |
Free forum by Nabble | Edit this page |