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

Wed Feb 4 21:17:47 UTC 2015

Hi Stephen,

That covers the cases better.

The updated javadoc is:
http://cr.openjdk.java.net/~rriggs/xx/java/time/chrono/Chronology.html
http://cr.openjdk.java.net/~rriggs/xx/java/time/chrono/HijrahChronology.html

Webrev:
    http://cr.openjdk.java.net/~rriggs/webrev-leap-year-8067800/

Roger

On 2/4/2015 3:42 PM, Stephen Colebourne wrote:
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>