On Mon, Sep 7, 2015 at 9:32 AM, Jochen Theodorou <[email protected]> wrote: > Am 03.09.2015 18:29, schrieb Thibault Kruse: >> >> So the anti-commutativity is broken in my example because >> NumberAwareComparator because DefaultTypeTransformation.compareTo does >> not perform symmetric checks (only 'left' needs to implement >> Comparable, not 'right'). >> >> Maybe this could already be fixed by >> DefaultTypeTransformation.compareTo requiring both arguments to be of >> equivalent type, such as both being Comparables. And possibly as an >> alternative fallback both being of the same class and having a >> compareTo Method (which would break less groovy code I guess). > > > DefaultTypeTransformation.compareTo does something like that actually, but > not for numbers. And if you want to compare Double and Integer, it won't > work that way.
I was not sure that it does, looking at the code, so I looked to extend the unit test. I found it has no unit test. Finding this makes me sad. Anyway, I wrote some tests, to find there are several issues with symmetry: https://github.com/tkruse/incubator-groovy/commit/aa8d201cbbca6823adbacf15cddfc7dd68881a13 Given there were no tests, I am not too surprised to find bugs. So I would recommend fixing those, then adding some more tests with other Comparables or Objects with compareTo, and make sure that it behaves symmetrically and consistently. >> This could further be relaxed to also allow comparing two instances >> *of the same class* when they have a compareTo method. But comparing a >> MyNumber with an Integer should IMO fail in >> DefaultTypeTransformation.compareTo(). >> >> Another problem of anti-commutativity arises due to the weird >> hashCode() comparison that never returns 0, even when both hashCode >> values are the same. >> Maybe it would be better to compare the System hashcodes >> System.identityHashCode(x) rather than the hashcodes from the objects. >> Maybe that way even a 0 value could be returned then. > > > if both objects have referential identity > DefaultTypeTransformation.compareTo, will return 0. The method will also > handle null. What should we do if the hashcodes are the same, but the > objects are not? identityHashCode gives no guaranteee I see. So probably better not to have any fallback in NumberAwareComparator. But I cannot say whether in general it would be acceptable to Groovy users if a new version of Groovy started throwing exceptions some unhealthy cases of comparing Objects that should not be compared (or should implement Comparable, or should use a custom Comparator). On Mon, Sep 7, 2015 at 9:32 AM, Jochen Theodorou <[email protected]> wrote: > Am 03.09.2015 18:29, schrieb Thibault Kruse: >> >> So the anti-commutativity is broken in my example because >> NumberAwareComparator because DefaultTypeTransformation.compareTo does >> not perform symmetric checks (only 'left' needs to implement >> Comparable, not 'right'). >> >> Maybe this could already be fixed by >> DefaultTypeTransformation.compareTo requiring both arguments to be of >> equivalent type, such as both being Comparables. And possibly as an >> alternative fallback both being of the same class and having a >> compareTo Method (which would break less groovy code I guess). > > > DefaultTypeTransformation.compareTo does something like that actually, but > not for numbers. And if you want to compare Double and Integer, it won't > work that way. > >> This could further be relaxed to also allow comparing two instances >> *of the same class* when they have a compareTo method. But comparing a >> MyNumber with an Integer should IMO fail in >> DefaultTypeTransformation.compareTo(). >> >> Another problem of anti-commutativity arises due to the weird >> hashCode() comparison that never returns 0, even when both hashCode >> values are the same. >> Maybe it would be better to compare the System hashcodes >> System.identityHashCode(x) rather than the hashcodes from the objects. >> Maybe that way even a 0 value could be returned then. > > > if both objects have referential identity > DefaultTypeTransformation.compareTo, will return 0. The method will also > handle null. What should we do if the hashcodes are the same, but the > objects are not? identityHashCode gives no guaranteee > >> Whether it makes sense at all to compare objects that way I don't know. >> >> Comparing 0.0d and 0 as same without breaking equals-consistency seems >> impossible, not sure what to do about that. > > > it is worse. compare 1.0G and 1.00G. BigDecimal.compare will say the are > euqal, BigDecimal.equals will say no. > > > bye blackdrag > > -- > Jochen "blackdrag" Theodorou > blog: http://blackdragsview.blogspot.com/ >
