Review Request for project Threeten updates

[roger riggs]

Hi Sherman,

On 10/3/2013 2:38 PM, Xueming Shen wrote:
>
> (1) until(Temporal endExclusive, TEmporalUnit unit)
>     -> Shouldn't we invoke requireNonNull() for unit before invoking 
> unit.between(...)?
There is no doubt that invoking unit.between() will cause an NPE if unit 
is null.
Adding requireNonNull will only improve the error message and add bulk 
to the code.
>
> (2) It appears we started to use "endExclusive" in "until() methods, 
> while the param
>      has been renamed to be "exclusive" explicitly, personally I 
> prefer still keep the
>      word "exclusive" in its spec, or at least we want to be 
> consistent, for example
>      LocalDate.until(ChronoLocalDate) still keeps the "exclusive" 
> after renaming.
The wording of the @param tags were not changed.  We could add ", 
exclusive, "
to the @param tags.
>
> (3) We have the requireNonNull check pattern below in most places
>     if (X instanceof Y) { ... }
>     Objects.requireNonNull(X, "x");
Isn't that just useless, instanceof only returns true for non-null argument.
>
>     but the plus/minus(TemporalAmount amountToSubtract) appears to be
>
>     Objects.requireNonNull(...)
>      if (X instanceof ...) {...}
This form would identify the null as coming from the parameter but the 
null check would
be performed twice (unless HS optimized)
>
> (4) spec and impl of the until(end, unit) has been updated regarding 
> passing the
>     "converted" input temporal as the second argument for 
> unit.between(...)
>
>     If the unit is not a {@code ChronoUnit}, then the result of this 
> method
>     is obtained by invoking {@code TemporalUnit.between(Temporal, 
> Temporal)}
>     passing {@code this} as the first argument and the converted input 
> temporal
>     as the second argument.
>     I do see the TemporalUnit.between() has wording regarding the 
> first and second must be
>     the same "type/class", otherwise, it will send the ball back to 
> Temporal,  then we might
>     end up with a loop. My guess these new wording is to prevent a 
> possible implementation
>     deadlock? But it appears the super Temporal/ChronoLocalXYZ 
> interfaces still claim
>     the "input temporal as the second argument", though it also 
> insists that
That's an oversight and is inconsistent with the behavior defined by the 
pseudocode.
It should say "converted input temporal" as it does in other places.
Both Temporal.until and ChronoLocalDate.until(Temporal...) should be 
corrected.
>
>     "Note that the unit's {@code between} method must only be invoked 
> if two temporal
>     objects have exactly the same type evaluated by {@code getClass()}."
>
>     and the sample shows it indeed passes the converted one. just a 
> small typo here?
yes
>
>     (The ChronoLocalXYZImpl.until(end, unit) probably needs to update 
> the api
>     doc to explicitly describe this change as well, instead of 
> "@Override", if the wording
>     in Temporal/ChronoLocalXYz is indeed by design?)
When corrected, it should apply equally to ChronoXXXX.

Thanks, Roger

>
> -Sherman
>
> On 10/01/2013 11:26 AM, roger riggs wrote:
>> Please review these changes from the Threeten project for integration 
>> into jdk-tl.
>>
>> http://cr.openjdk.java.net/~rriggs/webrev-period-until-8023762-807-834-835/ 
>>
>>
>> 8023762: Add ChronoPeriod interface and bind period to Chronology
>> Summary: Make Period ISO-only, adding a Chronology-specific period 
>> concept
>> Contributed-by: scolebourne at joda.org
>>
>> 8023763: Rename ChronoDateImpl
>> Summary: Rename ChronoDateImpl to ChronoLocalDateImpl
>> Contributed-by: scolebourne at joda.org
>>
>> 8023764: Optimize Period addition
>> Summary: Optimise plus/minus for common cases
>> Contributed-by: scolebourne at joda.org
>>
>> 8024835: Change until() to accept any compatible temporal
>> Summary: Method until(Temporal,TemporalUnit) now uses from() to 
>> convert; Enhance from() methods where necessary
>> Contributed-by: scolebourne at joda.org
>>
>> 8024807: Add getChronlogy() to CLDT/CZDT
>> Summary: Alternative to method is clunky and hard to find
>> Contributed-by: scolebourne at joda.org
>>
>> 8024834: Better return type for TemporalField resolve
>> Summary: Allow resolve method to return more than just ChronoLocalDate
>> Contributed-by: scolebourne at joda.org
>>
>> 8024999: Instant.Parse typo in example
>> Summary: javadoc only fix to correct example to use "." and "Z"
>>
>> Thanks, Roger
>>
>>
>>
>>
>