(1) until(Temporal endExclusive, TEmporalUnit unit)
    -> Shouldn't we invoke requireNonNull() for unit before invoking 
unit.between(...)?

(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.

(3) We have the erquireNonNull check pattern below in most places
    if (X instanceof Y) { ... }
    Objects.requireNonNull(X, "x");

    but the plus/minus(TemporalAmount amountToSubtract) appears to be

    Objects.requireNonoNull(...)
     if (X instanceof ...) {...}

(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

    "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?

    (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?)

-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: scolebou...@joda.org

8023763: Rename ChronoDateImpl
Summary: Rename ChronoDateImpl to ChronoLocalDateImpl
Contributed-by: scolebou...@joda.org

8023764: Optimize Period addition
Summary: Optimise plus/minus for common cases
Contributed-by: scolebou...@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: scolebou...@joda.org

8024807: Add getChronlogy() to CLDT/CZDT
Summary: Alternative to method is clunky and hard to find
Contributed-by: scolebou...@joda.org

8024834: Better return type for TemporalField resolve
Summary: Allow resolve method to return more than just ChronoLocalDate
Contributed-by: scolebou...@joda.org

8024999: Instant.Parse typo in example
Summary: javadoc only fix to correct example to use "." and "Z"

Thanks, Roger





Reply via email to