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: 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
