Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Looks fine Ivan. Reviewed. Regards, Sean. On 19/08/16 15:59, Ivan Gerasimov wrote: On 19.08.2016 17:32, Stephen Colebourne wrote: I'm less comfortable with the compareTo change because it may make it slower and that may have knock on effects. I think a comment saying that both are bounded would be enough in compareTo() Okay. I reverted that change back then and added a comment. The webrev is updated: http://cr.openjdk.java.net/~igerasim/8164366/02/webrev/ With kind regards, Ivan Stephen On 19 August 2016 at 13:52, Ivan Gerasimov wrote: Thanks Nadeesh. It's a good catch! Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8164366/01/webrev/ I also slightly modified comareTo(), not because there was an error in it, but just to avoid thinking too much about possible overflow in subtraction (of course, there can be no overflow here, as totalSeconds is bounded.) Now we just need official blessing from the Reviewer. With kind regards, Ivan On 19.08.2016 9:01, nadeesh tv wrote: Hi Ivan, ZoneOffset.ofTotalSeconds(Integer.MIN_VALUE) have also the same issue. It' not throwing the expected DTE. May be you can correct this also as part of this. Please update the copyright year also. Rest looks good.
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
On 19.08.2016 17:32, Stephen Colebourne wrote: I'm less comfortable with the compareTo change because it may make it slower and that may have knock on effects. I think a comment saying that both are bounded would be enough in compareTo() Okay. I reverted that change back then and added a comment. The webrev is updated: http://cr.openjdk.java.net/~igerasim/8164366/02/webrev/ With kind regards, Ivan Stephen On 19 August 2016 at 13:52, Ivan Gerasimov wrote: Thanks Nadeesh. It's a good catch! Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8164366/01/webrev/ I also slightly modified comareTo(), not because there was an error in it, but just to avoid thinking too much about possible overflow in subtraction (of course, there can be no overflow here, as totalSeconds is bounded.) Now we just need official blessing from the Reviewer. With kind regards, Ivan On 19.08.2016 9:01, nadeesh tv wrote: Hi Ivan, ZoneOffset.ofTotalSeconds(Integer.MIN_VALUE) have also the same issue. It' not throwing the expected DTE. May be you can correct this also as part of this. Please update the copyright year also. Rest looks good.
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
I'm less comfortable with the compareTo change because it may make it slower and that may have knock on effects. I think a comment saying that both are bounded would be enough in compareTo() Stephen On 19 August 2016 at 13:52, Ivan Gerasimov wrote: > Thanks Nadeesh. It's a good catch! > > Here's the updated webrev: > http://cr.openjdk.java.net/~igerasim/8164366/01/webrev/ > > I also slightly modified comareTo(), not because there was an error in it, > but just to avoid thinking too much about possible overflow in subtraction > (of course, there can be no overflow here, as totalSeconds is bounded.) > > Now we just need official blessing from the Reviewer. > > With kind regards, > Ivan > > > > On 19.08.2016 9:01, nadeesh tv wrote: >> >> Hi Ivan, >> >> ZoneOffset.ofTotalSeconds(Integer.MIN_VALUE) have also the same issue. It' >> not throwing the expected DTE. >> >> May be you can correct this also as part of this. >> >> Please update the copyright year also. >> >> Rest looks good. >> >
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Thanks Nadeesh. It's a good catch! Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8164366/01/webrev/ I also slightly modified comareTo(), not because there was an error in it, but just to avoid thinking too much about possible overflow in subtraction (of course, there can be no overflow here, as totalSeconds is bounded.) Now we just need official blessing from the Reviewer. With kind regards, Ivan On 19.08.2016 9:01, nadeesh tv wrote: Hi Ivan, ZoneOffset.ofTotalSeconds(Integer.MIN_VALUE) have also the same issue. It' not throwing the expected DTE. May be you can correct this also as part of this. Please update the copyright year also. Rest looks good.
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Hi Ivan, ZoneOffset.ofTotalSeconds(Integer.MIN_VALUE) have also the same issue. It' not throwing the expected DTE. May be you can correct this also as part of this. Please update the copyright year also. Rest looks good. -- Thanks and Regards, Nadeesh TV On 8/18/2016 10:23 PM, Ivan Gerasimov wrote: Hello everybody! The factory methods of ZoneOffset class demonstrate a minor inconsistency. Normally, invalid values of arguments are rejected (e.g. when minutes > 59), but the value of Integer.MIN_VALUE is allowed to be passed in. This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366 WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/ With kind regards, Ivan -- Thanks and Regards, Nadeesh TV
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Hi , Sorry , my bad. I misread 'plusmn'. -- Thanks and Regards, Nadeesh TV On 8/19/2016 11:10 AM, nadeesh tv wrote: Hi Stephen/Ivan, Is not the the statement "Zone offset minutes and seconds must be negative because hours is negative" and the following doc definition contradicts ? 358 * @param minutes the time-zone offset in minutes, from 0 to ±59 359 * @param seconds the time-zone offset in seconds, from 0 to ±59 -- Thanks and Regards, Nadeesh TV
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Hi Stephen/Ivan, Is not the the statement "Zone offset minutes and seconds must be negative because hours is negative" and the following doc definition contradicts ? 358 * @param minutes the time-zone offset in minutes, from 0 to ±59 359 * @param seconds the time-zone offset in seconds, from 0 to ±59 -- Thanks and Regards, Nadeesh TV On 8/18/2016 11:46 PM, Stephen Colebourne wrote: Looks good Stephen On 18 August 2016 at 17:53, Ivan Gerasimov wrote: Hello everybody! The factory methods of ZoneOffset class demonstrate a minor inconsistency. Normally, invalid values of arguments are rejected (e.g. when minutes > 59), but the value of Integer.MIN_VALUE is allowed to be passed in. This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366 WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/ With kind regards, Ivan
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Thank you Stephen for review! On 18.08.2016 21:16, Stephen Colebourne wrote: Looks good Stephen On 18 August 2016 at 17:53, Ivan Gerasimov wrote: Hello everybody! The factory methods of ZoneOffset class demonstrate a minor inconsistency. Normally, invalid values of arguments are rejected (e.g. when minutes > 59), but the value of Integer.MIN_VALUE is allowed to be passed in. This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366 WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/ With kind regards, Ivan
Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Looks good Stephen On 18 August 2016 at 17:53, Ivan Gerasimov wrote: > Hello everybody! > > The factory methods of ZoneOffset class demonstrate a minor inconsistency. > Normally, invalid values of arguments are rejected (e.g. when minutes > 59), > but the value of Integer.MIN_VALUE is allowed to be passed in. > > This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well. > > Would you please help review the fix? > > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366 > WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/ > > With kind regards, > Ivan >
[jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input
Hello everybody! The factory methods of ZoneOffset class demonstrate a minor inconsistency. Normally, invalid values of arguments are rejected (e.g. when minutes > 59), but the value of Integer.MIN_VALUE is allowed to be passed in. This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366 WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/ With kind regards, Ivan
