looks fine.
-sherman
On 10/4/13 7:45 AM, roger riggs wrote:
The webrev has been updated to:
- In the until methods, add exclusive to the descriptions in the
@param tags in addition to the parameter name
- Optimize the use of requireNonNull so it appears only in the paths
for instanceof == false
- Clarified in Temporal.until that the *converted* input temporal is
passed to the between method.
http://cr.openjdk.java.net/~rriggs/webrev-period-until-8023762-807-834-835/
Thanks, Roger
On 10/4/2013 6:16 AM, Stephen Colebourne wrote:
On 3 October 2013 23:08, Xueming Shen <xueming.s...@oracle.com> wrote:
(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 "exclusive" in the parameter name is for IDE autocompletion. I'm
surprised I didn't put it in before as its my long standing practice
and very useful for users. The Javadoc should also state exclusive.
On 10/03/2013 02:03 PM, roger riggs wrote:
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.
Most of the use cases of requireNonNull here are "no doubt that a
NPE will
be triggered"
and the only benefit of having them is to improve the err msg and
there for
add bulk to
the code. What I'm saying here is the consistency.
There are lots of places in 310 where we have Objects.requireNotNull
and lots of places where we don't. Where we don't the minimum should
be an early throw of NPE that would be simple for a user to identify.
If it would be complex to identify then requireNotNull is useful.
So, not entirely consistent, but not completely inconsistent either...
(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.
Yes, that check is useless.
I may not have stated my observation clearly enough. In most of the
cases
the code inside {...} returned. My guess of intent of the original
code is
to
optimize the case of non-null parameter AND the X is the instance of Y,
which
should be normal use scenario. But the plus/minus implementation
appears
not try to optimize this (null always triggers false for instanceof)
for
case that
the amountToAdd is indeed a Period. This is also related my argument
in (1),
why we check requireNonNull here but not in (1). Well, not a big deal
though.
In general, if there is a sensible order of code for minor
optimisation I tried to do it. But it might not be consistent.
Stephen
