RFR 9: 8067800: Unexpected DateTimeException in the java.time.chrono.HijrahChronology.isLeapYear

Wed Feb 4 20:42:02 UTC 2015

I think this might be clearer:

* Checks if the specified year is a leap year.
* <p>
* A leap-year is a year of a longer length than normal.
* The exact meaning is determined by the chronology according to the
following constraints.
* <ul>
* <li>a leap-year must imply a year-length longer than a non leap-year.
* <li>a chronology that does not support the concept of a year must
return false.
* <li>the correct result must be returned for all years within the
valid range of years for the chronology
* </ul>
* Outside the range of valid years an implementation is free to return
either a best guess or false.
* An implementation must not throw an exception, even if the year is
outside the range of valid years..

Stephen

On 4 February 2015 at 19:08, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> Hi,
>
> Clarifying that Chronology.isLeapYear is specified only within the range of
> the chronology
> makes it possible to maintain the invariants with the calendar computations
> and methods.
> Best effort isn't testable except in an implementation specific way and
> can't be relied on.
>
> The other two constraints are testable and use 'must' in their definitions.
> The new phrasing should clearly be either an exception or reinforce the
> value is
> specified only within the range of the chronology.
>
> How about:
>
> <li>except if the year is out of range the chronology should return a best
> effort guess, or false if there is no suitable guess
>
> Roger
>
>
>
> On 2/4/2015 11:18 AM, Stephen Colebourne wrote:
>>
>> The spec is pretty clear:
>> "the proleptic-year to check, not validated for range"
>>
>> Adding an exception to the spec would cause requests to add exceptions
>> to all other chronologies. Doing so, would be very negative to
>> performance (it would require having a non-public copy of the logic
>> elsewhere to avoid the range checking cost.
>>
>> Adding an exception to Hijrah only means that the implementation is
>> doing something not permitted by the spec.
>>
>> The only sane choice here is to return false from Hijrah when out of
>> range. As intended and as specced.
>>
>> Note that I don't believe that returning false will cause hardship in
>> any actual user code, because other methods will constrain the year to
>> be valid.
>>
>> If necessary, the following spec clarification could be added to the
>> bullet points on Chronology:
>>
>> <li>if the year is out of range the chronology should return a best
>> effort guess, or false if there is no suitable guess
>>
>> Stephen
>>
>>
>> On 4 February 2015 at 16:05, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 2/4/15 10:43 AM, Stephen Colebourne wrote:
>>>>
>>>> The java.time code generally takes a lenient approach to methods that
>>>> return a boolean. For example, they tend to accept null inputs without
>>>> throwing an exception.
>>>
>>> Seems like an odd design provision leading to some behavior that
>>> would be inconsistent with non-boolean methods.
>>>>
>>>>
>>>> Right now, this patch makes a subclass implementation incompatible
>>>> with the superclass spec. That cannot happen. Thus there are only two
>>>> options:
>>>>
>>>> - change the superclass spec
>>>> - return false from the subclass
>>>>
>>>> I favour the latter as being in line with the rest of the package and
>>>> less disruptive to existing users (would a CCC even pass?)
>>>
>>> For a 'young' API with limited uptake, issues can be fixed early and in a
>>> major
>>> release there is more flexibility to clarify the specification.
>>>
>>> DTE is a runtime exception and can occur in a majority of time
>>> computations.
>>> In the case of HijrahChronolog.isLeapYear, the implementation currently
>>> throws a different RuntimeException and this would be a correction
>>> to the conventional exception.
>>>
>>> Roger
>>>
>>>
>>>> Stephen
>>>>
>>>>
>>>> On 4 February 2015 at 15:10, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> I also indicated in the Jira comments that it is misleading and
>>>>> incorrect
>>>>> to
>>>>> return
>>>>> false when it is not known that a year is or is not a leap year. All of
>>>>> the
>>>>> other
>>>>> HijrahChronology computations throw DTE for invalid dates and the same
>>>>> may
>>>>> be
>>>>> true for other Chronologies.
>>>>>
>>>>> The assertion in Chronology.isLeapYear about not checking the range is
>>>>> too
>>>>> broad
>>>>> and should be qualified by the Chronology.
>>>>>
>>>>> Perhaps the proposed change should include a caveat in that method.
>>>>>
>>>>> Roger
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 2/3/15 8:05 PM, Stephen Colebourne wrote:
>>>>>>
>>>>>> -1
>>>>>>
>>>>>> As I indicated on JIRA, I don't believe that this change meets the
>>>>>> spec or intent of the definition on Chronology. That is specified to
>>>>>> not throw any exceptions and to handle all years, valid or not.
>>>>>>
>>>>>> I don't foresee any significant issue where a year is not validated by
>>>>>> this method. Years out of range should simply return false, again
>>>>>> something that is within the spirit of the spec "a chronology that
>>>>>> does not support the concept of a year must return false."
>>>>>>
>>>>>> Stephen
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 3 February 2015 at 20:56, Lance Andersen
>>>>>> <lance.andersen at oracle.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> +1
>>>>>>> On Feb 3, 2015, at 3:45 PM, Roger Riggs <Roger.Riggs at oracle.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Please review this specification clarification of the range of
>>>>>>>> Hijrah
>>>>>>>> calendar variants.
>>>>>>>> The issue was exposed by a bug in the HijrahChronology.isLeapYear
>>>>>>>> method.
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>>      http://cr.openjdk.java.net/~rriggs/webrev-leap-year-8067800/
>>>>>>>>
>>>>>>>> Issue:
>>>>>>>>      https://bugs.openjdk.java.net/browse/JDK-8067800
>>>>>>>>
>>>>>>>> A CCC may be needed.
>>>>>>>>
>>>>>>>> Thanks, Roger
>>>>>>>>
>>>>>>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>>>>>> Oracle Java Engineering
>>>>>>> 1 Network Drive
>>>>>>> Burlington, MA 01803
>>>>>>> Lance.Andersen at oracle.com
>>>>>>>
>>>>>>>
>>>>>>>
>