Review Request for project Threeten updates

[roger riggs]

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.shen at 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