Re: RFR: 8283996: Reduce cost of year and month calculations

2022-03-30 Thread Stephen Colebourne
On Wed, 30 Mar 2022 12:06:39 GMT, Claes Redestad wrote: > A few integer divisions and multiplications can be replaced with test + > addition, leading to a decent speed-up on java.time microbenchmarks such as > `GetYearBench`. Numbers from my local x86 workstation, seeing similar > speed-up on

Re: RFR: 8283842: TestZoneTextPrinterParser.test_roundTripAtOverlap fails: DateTimeParseException

2022-03-30 Thread Stephen Colebourne
On Wed, 30 Mar 2022 16:46:40 GMT, Naoto Sato wrote: > Fixes test failures caused by depending on the default locale. Specifying > explicit `Locale.US` will do. Marked as reviewed by scolebourne (Author). - PR: https://git.openjdk.java.net/jdk/pull/8045

Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]

2022-05-11 Thread Stephen Colebourne
On Tue, 10 May 2022 17:43:07 GMT, Naoto Sato wrote: >> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43: >> >>> 41: private Object[][] testZoneOffsets() { >>> 42: return new Object[][] { >>> 43: {ZoneId.of("Z"), 0}, >> >> I know, `ZoneId.of()` should

Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]

2022-05-11 Thread Stephen Colebourne
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull reque

Currency updates in JDK 7

2009-08-23 Thread Stephen Colebourne
I've been reviewing the Javadoc for the JDK 7 Currency class changes (bug 6332666). http://download.java.net/jdk7/docs/api/java/util/Currency.html The updates appear to allow users the ability to override the built in currency data, as indicated in the class javadoc: "Users can supersede the Java

Re: Currency updates in JDK 7

2009-08-24 Thread Stephen Colebourne
Naoto Sato wrote: Stephen Colebourne wrote: Unfortunately, not all currencies are associated with countries. Some, like XAG for 'silver', XDR for 'IMF drawing rights' or XXX for 'no currency' have no matching country. Thus, this file format is unable to rep

Re: Review request : 7180362: RFE: Implement date cutover functionality for currency.properties file

2012-08-29 Thread Stephen Colebourne
I object to the implementation of this change, specifically the date format - "The format of date must be {@code '-MM-dd-HH-mm-ss'}" There seems to be no reason not to use the standard ISO-8601 format here - -MM-dd'T'HH:mm:ss Stephen co-spec lead JSR-310 On 28 August 2012 14:48, Seán Co

Re: Review request : 7180362: RFE: Implement date cutover functionality for currency.properties file

2012-09-07 Thread Stephen Colebourne
e tests >> to ensure all is ok. >> Updated webrev to follow. I'll also follow up with getting new change >> approved with the >> compatibility/conformance team. >> >> regards, >> Sean. >> >> On 29/08/2012 22:00, Stephen Colebourne wrote: &g

Re: [threeten-dev] Review request: Splitting locale resources (FormatData) for java.time classes

2013-04-03 Thread Stephen Colebourne
The changes sound sensible and I didn't see anything wrong in a quick look through. Stephen On 3 April 2013 17:09, Masayoshi Okutsu wrote: > Hi, > > I've made changes for splitting locale resources into FormatData required by > the legacy i18n classes and JavaTimeSupplementary required by the jav

Re: Support Hijri calendar

2013-10-09 Thread Stephen Colebourne
On 20 September 2013 15:25, Suhail Alkowaileet wrote: > I would like to support the lunar Hijri calendar(see: > https://en.wikipedia.org/wiki/Hijri ) to the jdk and I'm just asking should > I do something before doing this? > My plan is to clone the jdk8/l10n/jdk and add the appropriate files to o

Re: [9] RFR: 8048123: Replace calendars.properties with another mechanism to specify a new Japanese calendar era

2014-07-22 Thread Stephen Colebourne
It seems that the system property only supports one additional era. Given that Japanese eras are based on human lifetimes, that seems an invalid assumption. Stephen On 22 July 2014 08:06, Masayoshi Okutsu wrote: > Hello, > > Please review the change for JDK-8048123. This change removes all the er

Re: [9] RFR: 8048123: Replace calendars.properties with another mechanism to specify a new Japanese calendar era

2014-07-22 Thread Stephen Colebourne
support one additional era until > the era is added to the built-in eras in update releases. > > Masayoshi > > > On 7/22/2014 4:52 PM, Stephen Colebourne wrote: >> >> It seems that the system property only supports one additional era. >> Given that Japanese eras

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-09 Thread Stephen Colebourne
The logic looks fine. In the main code, this part .getLong(INSTANT_SECONDS); can be replaced with .toEpochSecond(); which will be slightly faster. In the test case, this part .plus(15, ChronoUnit.MINUTES); can be replaced with .plusMinutes(15) And .with(ChronoField.OFFSET_SECONDS, ZoneOff

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-10 Thread Stephen Colebourne
test cases as per inputs given by Stephen. Also, > I have added the javadocs changes in this patch which were proposed in the > bug. > > Bug link is: https://bugs.openjdk.java.net/browse/JDK-8066982 > > > Regards, > Ramanand. > > -Original Message- > From: Step

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread Stephen Colebourne
e without adding more confidence to > the quality. > > Thanks, Roger > > > > On 12/10/2015 11:00 AM, Stephen Colebourne wrote: >> >> I believe this is suitable for committing, thanks, other reviews welcome! >> Stephen >> >> >> >> On 10

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread Stephen Colebourne
e tests would be a bit cleaner > without it. > > - Typically, test that have data dependencies (such as the timezone > data) cannot be used for > compatibility to the specification, but the data is stable and it seems > unavoidable in this case. > > - Are all of t

Re: RFR: JDK-8147912: test "parseWithZoneWithoutOffset" of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on de_DE locale

2016-01-27 Thread Stephen Colebourne
>From the name of the test and looking at the code, running in English only is fine. Stephen On 27 January 2016 at 05:53, Masayoshi Okutsu wrote: > Looks OK to me. But I'd like some java.time people to review this change > to see if the intention of this test is to run only in English. > > Thank

Re: Request for Review : JDK-8071929 -Locale.getISOCountries() has inconsistent behaviour for "AN", "BU" and "CS" country codes.

2016-11-29 Thread Stephen Colebourne
I'm concerned that this is not the friendliest of new APIs. There is little description of the meaning of the ISO-3166 parts - what is being added is directly exposing the underlying data rather than providing any kind of abstraction. There is also an inconsistency between "ISO" and "Iso" in the c

Re: Request for Review : JDK-8071929 -Locale.getISOCountries() has inconsistent behaviour for "AN", "BU" and "CS" country codes.

2016-11-30 Thread Stephen Colebourne
useful would be a much more detailed result, where a user could lookup an old code and find out when it expired, and what replaced it. Stephen On 30 November 2016 at 09:31, Rachna Goel wrote: > Hi Stephen, > > Thanks a lot for the review. > > On 30/11/16 3:15 AM, Stephen Colebourne

Re: CLDR Irish time zone name

2018-02-13 Thread Stephen Colebourne
On 13 February 2018 at 22:20, Yoshito Umaoka wrote: > Hello Sato-san, > > As you know, tz database 2018a flipped Europe/Dublin winter/summer time > rules. There were many messages posted in the tz mailing list, and Paul > Eggert decided to revert the change in tz database 2018c. For future > relea

Re: CLDR Irish time zone name

2018-02-14 Thread Stephen Colebourne
On 13 February 2018 at 23:25, Martin Buchholz wrote: > On Tue, Feb 13, 2018 at 3:02 PM, Stephen Colebourne >> If TZDB insists on continuing with the change, the TZDB compiler in >> OpenJDK will have to be changed. I've managed to make a change (hack) >> to the c

Re: RFR: 8247781: Day periods support

2020-10-30 Thread Stephen Colebourne
On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato wrote: > Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > m

Re: RFR: 8247781: Day periods support [v3]

2020-11-02 Thread Stephen Colebourne
On Fri, 30 Oct 2020 11:06:59 GMT, Stephen Colebourne wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed exception messages. > > src/java.base/share/classes/java/time/format/P

Re: RFR: 8247781: Day periods support [v3]

2020-11-02 Thread Stephen Colebourne
On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder

Re: RFR: 8247781: Day periods support [v3]

2020-11-02 Thread Stephen Colebourne
On Fri, 30 Oct 2020 21:30:50 GMT, Naoto Sato wrote: >> src/java.base/share/classes/java/time/format/Parsed.java line 472: >> >>> 470: } >>> 471: if (dayPeriod != null) { >>> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) { >> >> Are we certain that the CLDR data does

Re: RFR: 8247781: Day periods support [v7]

2020-11-05 Thread Stephen Colebourne
On Thu, 5 Nov 2020 17:12:11 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder

Re: RFR: 8247781: Day periods support [v7]

2020-11-05 Thread Stephen Colebourne
On Mon, 2 Nov 2020 23:21:22 GMT, Naoto Sato wrote: >> Pulling on this a little more. >> >> As the PR stands, it seems that if the user passes in text with just a >> day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in >> `AMPM_OF_DAY` of "AM" the get no `LocalTime` in the res

Re: RFR: 8247781: Day periods support [v7]

2020-11-06 Thread Stephen Colebourne
On Fri, 6 Nov 2020 03:00:52 GMT, Naoto Sato wrote: >> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line >> 858: >> >>> 856: return new Object[][]{ >>> 857: {STRICT, 0, LocalTime.of(6, 0), 0}, >>> 858: {STRICT, 1, LocalTime.of(18,

Re: RFR: 8247781: Day periods support [v9]

2020-11-06 Thread Stephen Colebourne
On Thu, 5 Nov 2020 23:24:19 GMT, Stephen Colebourne wrote: >> I implemented what you suggested here in the latest PR, but that would be a >> behavioral change which requires a CSR, as "AM" would be resolved to 06:00 >> which was not before. Do you think it would

Re: RFR: 8247781: Day periods support [v12]

2020-11-12 Thread Stephen Colebourne
On Tue, 10 Nov 2020 19:52:14 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder

Re: RFR: 8247781: Day periods support [v13]

2020-11-12 Thread Stephen Colebourne
On Thu, 12 Nov 2020 20:03:14 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder

Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v7]

2021-04-21 Thread Stephen Colebourne
On Wed, 21 Apr 2021 11:06:16 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request w

Re: RFR: 8268469: Update java.time to use switch expressions

2021-06-09 Thread Stephen Colebourne
On Wed, 9 Jun 2021 15:41:59 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.time` > packages to make use of the switch expressions? > > Kind regards, > Patrick My biggest comment is the spaces used to align, which I strongly obje

Re: RFR: 8268469: Update java.time to use switch expressions [v3]

2021-06-16 Thread Stephen Colebourne
On Wed, 16 Jun 2021 10:57:07 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new

Re: RFR: 8268469: Update java.time to use switch expressions [v3]

2021-06-16 Thread Stephen Colebourne
On Wed, 16 Jun 2021 11:11:30 GMT, Chris Hegarty wrote: >> src/java.base/share/classes/java/time/Month.java line 480: >> >>> 478: int leap = leapYear ? 1 : 0; >>> 479: return switch (this) { >>> 480: case JANUARY -> 1; >> >> Unnecessary alignment > > The vertical al

Re: RFR: 8268469: Update java.time to use switch expressions [v3]

2021-06-17 Thread Stephen Colebourne
On Thu, 17 Jun 2021 13:56:00 GMT, Daniel Fuchs wrote: >> It is your codebase, not mine, so it is up to you. Aligning things by column >> is generally frowned on in most style guides because it handles refactoring >> poorly, resulting in lots of needless change (or people forgetting to >> reali

Re: RFR: 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong

2021-08-26 Thread Stephen Colebourne
On Mon, 23 Aug 2021 16:42:03 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. When instant seconds and zone > co-exist in parsed data, instant seconds was not resolved correctly from them. LGTM - Marked as reviewed by scolebourne (Author). PR: https://git.open

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter

2021-11-01 Thread Stephen Colebourne
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad wrote: > Prompted by a request from Volkan Yazıcı I took a look at why the java.time > formatters are less efficient for some common patterns than custom formatters > in apache-commons and log4j. This patch reduces the gap, without having > looke

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]

2021-11-02 Thread Stephen Colebourne
On Mon, 1 Nov 2021 22:35:58 GMT, Claes Redestad wrote: >> The commentary on this line could probably be improved, but this is in a >> private printer-parser that will only be used for NANO_OF_SECOND and not any >> arbitrary `TemporalField` (see line 704), thus I fail to see how this >> assumpt

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]

2021-11-03 Thread Stephen Colebourne
On Tue, 2 Nov 2021 11:03:02 GMT, Claes Redestad wrote: >> Prompted by a request from Volkan Yazıcı I took a look at why the java.time >> formatters are less efficient for some common patterns than custom >> formatters in apache-commons and log4j. This patch reduces the gap, without >> having l

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]

2021-11-03 Thread Stephen Colebourne
On Wed, 3 Nov 2021 13:14:42 GMT, Claes Redestad wrote: >> Prompted by a request from Volkan Yazıcı I took a look at why the java.time >> formatters are less efficient for some common patterns than custom >> formatters in apache-commons and log4j. This patch reduces the gap, without >> having l

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]

2021-11-09 Thread Stephen Colebourne
On Wed, 3 Nov 2021 22:55:23 GMT, Claes Redestad wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove accidentally committed experimental @Stable (no effect on micros) > > Thanks, Naoto! @cl4es For `DateTimeFor

Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v3]

2021-11-25 Thread Stephen Colebourne
On Wed, 24 Nov 2021 23:25:22 GMT, Naoto Sato wrote: >> This fix intends to honor the type (std/dst/generic) of parsed zone name for >> selecting the offset at the overlap. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with two additional > co

Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v4]

2021-12-01 Thread Stephen Colebourne
On Mon, 29 Nov 2021 17:41:34 GMT, Naoto Sato wrote: >> This fix intends to honor the type (std/dst/generic) of parsed zone name for >> selecting the offset at the overlap. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > co

Re: RFR: 8282081: java.time.DateTimeFormatter: wrong definition of symbol F

2022-02-28 Thread Stephen Colebourne
On Mon, 28 Feb 2022 23:17:57 GMT, Naoto Sato wrote: > Fixing the definition and implementation of the pattern symbol `F`. Although > it is an incompatible change, I believe it is worth the fix. For that, a CSR > has been drafted. Although there is incompatibility, I believe a fix is the correc

Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v3]

2022-03-06 Thread Stephen Colebourne
On Fri, 4 Mar 2022 23:05:56 GMT, Naoto Sato wrote: >> Supporting `IsoFields` temporal fields in chronologies that are similar to >> ISO chronology. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revi

Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v3]

2022-03-07 Thread Stephen Colebourne
On Fri, 4 Mar 2022 21:17:50 GMT, Jim Laskey wrote: >> Several attempts have been made to improve Formatter's numeric performance >> by caching the current Locale zero. Such fixes, however, ignore the real >> issue, which is the slowness of fetching DecimalFormatSymbols. By directly >> caching