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.ri...@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.ri...@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.ri...@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.ander...@oracle.com>
wrote:
+1
On Feb 3, 2015, at 3:45 PM, Roger Riggs <roger.ri...@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.ander...@oracle.com
