Review Request for project Threeten updates

[Xueming Shen]

On 10/03/2013 02:03 PM, roger riggs wrote:
> 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.

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.

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

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.

-Sherman