Review Request for project Threeten updates

[Xueming Shen]

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