[threeten-dev] Please review INSTANT_COMPARATOR fix

[roger riggs]

Hi Stephen,

A followup while examining the OffsetDateTime comparator...

The compareTo method contains an optimization for the offsets being equal.
Should that optimization also be in INSTANT_COMPARATOR?

With lambda syntax for method references, I created a static 2 arg method
for compareInstant and used it for the INSTANT_COMPARATOR
and in the compareTo method avoiding duplicate code.

In your opinion would it be useful to make the static comparator method 
public,
perhaps instead of the INSTANT_COMPARATOR field.

http://cr.openjdk.java.net/~rriggs/webrev-instant-comparator-8014939/

Also, to the previous comment, should the use of Long.compare be changed
to arithmetic subtraction using the previous argument?

Thanks, Roger



On 5/22/2013 4:14 PM, Stephen Colebourne wrote:
> While I think the fix is correct (as the code looks wrong) the comment
> "compare and INSTANT_COMPARATOR should be the same" is incorrect. The
> whole point of the instant comparator is that it isn't the same as the
> compare method. The test is really about testing nanos when epoch-secs
> are the same.
>
> The other changes to Integer.compare should be reverted. They make
> comparisons slower given that in all cases we know that the
> calculation will not overflow.
>
> Stephen
>
>
> On 22 May 2013 20:53, roger riggs <roger.riggs at oracle.com> wrote:
>> Hi,
>>
>> Please review this fix for the issue Patrick found [1] with
>> OfffSetDateTime.INSTANT_COMPARATOR.
>>
>> http://cr.openjdk.java.net/~rriggs/webrev-instant-comparator-8014939/
>>
>> I also noticed that the compareTo methods inconsistently use direct
>> arithmetic
>> and Integer.compare to compute the return values.  The webrev includes
>> updates to consistently use Integer.compare.
>>
>> Thanks, Roger
>>
>> [1] http://mail.openjdk.java.net/pipermail/threeten-dev/2013-May/001401.html
>>