Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v4]

2023-04-21 Thread Naoto Sato 
On Tue, 28 Feb 2023 07:49:17 GMT, Madjosz  wrote:

>> Fixes JDK-8302983 (and duplicate JDK-8302898)
>
> Madjosz has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   whitespace, remove stream()

Marked as reviewed by naoto (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/12690#pullrequestreview-1396233556

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v4]

2023-04-21 Thread Madjosz 
On Tue, 28 Feb 2023 07:49:17 GMT, Madjosz  wrote:

>> Fixes JDK-8302983 (and duplicate JDK-8302898)
>
> Madjosz has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   whitespace, remove stream()

Is this ready to merge now?

-

PR Comment: https://git.openjdk.org/jdk/pull/12690#issuecomment-1517466596

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v4]

2023-02-27 Thread Madjosz 
> Fixes JDK-8302983 (and duplicate JDK-8302898)

Madjosz has updated the pull request incrementally with one additional commit 
since the last revision:

  whitespace, remove stream()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12690/files
  - new: https://git.openjdk.org/jdk/pull/12690/files/1bcf7bd8..fa728668

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=02-03

  Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/12690.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12690/head:pull/12690

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v3]

2023-02-25 Thread Andrey Turbanov 
On Sat, 25 Feb 2023 00:07:45 GMT, Madjosz  wrote:

>> Fixes JDK-8302983 (and duplicate JDK-8302898)
>
> Madjosz has refreshed the contents of this pull request, and previous commits 
> have been removed. The incremental views will show differences compared to 
> the previous content of the PR. The pull request contains one new commit 
> since the last revision:
> 
>   apply review comments

src/java.base/share/classes/java/time/zone/ZoneRulesProvider.java line 319:

> 317: ZoneRulesProvider old = ZONES.putIfAbsent(zoneId, provider);
> 318: if (old != null) {
> 319: if(!old.equals(provider)) {

Suggestion:

if (!old.equals(provider)) {

src/java.base/share/classes/java/time/zone/ZoneRulesProvider.java line 322:

> 320: // restore old state
> 321: ZONES.put(zoneId, old);
> 322: provider.provideZoneIds().stream()

Can we drop `.stream()` call and just call `.forEach` directly?

-

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v3]

2023-02-24 Thread Madjosz 
> Fixes JDK-8302983 (and duplicate JDK-8302898)

Madjosz has refreshed the contents of this pull request, and previous commits 
have been removed. The incremental views will show differences compared to the 
previous content of the PR. The pull request contains one new commit since the 
last revision:

  apply review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12690/files
  - new: https://git.openjdk.org/jdk/pull/12690/files/0c7e5889..1bcf7bd8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/12690.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12690/head:pull/12690

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v2]

2023-02-24 Thread Naoto Sato 
On Fri, 24 Feb 2023 19:52:59 GMT, Madjosz  wrote:

>> Fixes JDK-8302983 (and duplicate JDK-8302898)
>
> Madjosz has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   apply review comments

Changes requested by naoto (Reviewer).

test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 107:

> 105: 
> 106: // availability check
> 107: assertTrue(ZoneId.getAvailableZoneIds().contains(zone), 
> "Unexpected non-availability for " + id);

Did you actually run the test??? It still does not compile. Please make sure 
you run the test under the modified jdk and passes.


STDERR:

-

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v2]

2023-02-24 Thread Madjosz 
On Fri, 24 Feb 2023 19:35:29 GMT, Madjosz  wrote:

>> test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 123:
>> 
>>> 121: ZoneRulesProvider.registerProvider(provider);
>>> 122: assertTrue(ZoneId.getAvailableZoneIds().contains(zone), 
>>> "Unexpected non-availability for " + zone);
>>> 123: assertNotNull(ZoneId.of(zone), "ZoneId instance for " + zone + 
>>> " should be obtainable");
>> 
>> If the `zone` does not exist, it will not return `null` but throw an 
>> exception. Assertion needs to be modified correctly.
>
> Which version of TestNG is used for JDK? In JUnit 5 I would just use 
> `assertDoesNotThrow()` in such a case but TestNG seems to lack such method.

I just removed the non-`null` assertion as the fact that the call succeeds 
already is the thing we want to test here.

-

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider [v2]

2023-02-24 Thread Madjosz 
> Fixes JDK-8302983 (and duplicate JDK-8302898)

Madjosz has updated the pull request incrementally with one additional commit 
since the last revision:

  apply review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12690/files
  - new: https://git.openjdk.org/jdk/pull/12690/files/e87cfef8..0c7e5889

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=00-01

  Stats: 71 lines in 1 file changed: 31 ins; 33 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/12690.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12690/head:pull/12690

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider

2023-02-24 Thread Madjosz 
On Wed, 22 Feb 2023 20:41:07 GMT, Naoto Sato  wrote:

>> Fixes JDK-8302983 (and duplicate JDK-8302898)
>
> test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 123:
> 
>> 121: ZoneRulesProvider.registerProvider(provider);
>> 122: assertTrue(ZoneId.getAvailableZoneIds().contains(zone), 
>> "Unexpected non-availability for " + zone);
>> 123: assertNotNull(ZoneId.of(zone), "ZoneId instance for " + zone + 
>> " should be obtainable");
> 
> If the `zone` does not exist, it will not return `null` but throw an 
> exception. Assertion needs to be modified correctly.

Which version of TestNG is used for JDK? In JUnit 5 I would just use 
`assertDoesNotThrow()` in such a case but TestNG seems to lack such method.

-

PR: https://git.openjdk.org/jdk/pull/12690

Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider

2023-02-22 Thread Naoto Sato 
On Tue, 21 Feb 2023 13:44:52 GMT, Madjosz  wrote:

> Fixes JDK-8302983 (and duplicate JDK-8302898)

Thanks for fixing this. Some comments follow.

test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 43:

> 41: /**
> 42:  * @summary Tests for ZoneRulesProvider class.
> 43:  * @bug 8299571

Can be collapsed into one line

test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 119:

> 117: return null;
> 118: }
> 119: }

Needs a semi-colon, otherwise would not compile.
Anyway, this inner class can be combined with the one in the test above, and 
made into a separate class.

test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 123:

> 121: ZoneRulesProvider.registerProvider(provider);
> 122: assertTrue(ZoneId.getAvailableZoneIds().contains(zone), 
> "Unexpected non-availability for " + zone);
> 123: assertNotNull(ZoneId.of(zone), "ZoneId instance for " + zone + " 
> should be obtainable");

If the `zone` does not exist, it will not return `null` but throw an exception. 
Assertion needs to be modified correctly.

test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 136:

> 134: // instantiation check
> 135: try {
> 136: assertNotNull(ZoneId.of(zone), "ZoneId instance for " + zone 
> + " should still be obtainable");

Same here.

-

Changes requested by naoto (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12690

RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider

2023-02-21 Thread Madjosz 
Fixes JDK-8302983 (and duplicate JDK-8302898)

-

Commit messages:
 - 8302983: ZoneRulesProvider.registerProvider() twice will remove provider

Changes: https://git.openjdk.org/jdk/pull/12690/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12690&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8302983
  Stats: 53 lines in 2 files changed: 49 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/12690.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12690/head:pull/12690

PR: https://git.openjdk.org/jdk/pull/12690