groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thibault Kruse <tibokr...@googlemail.com>
Subject Re: usage of NumberAwareComparator in extension methods and number oddities (a bit code review)
Date Mon, 07 Sep 2015 10:08:26 GMT
On Mon, Sep 7, 2015 at 9:32 AM, Jochen Theodorou <blackdrag@gmx.org> 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 <blackdrag@gmx.org> 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/
>

Mime
View raw message