ZoneRules.of() implies that transitionList is a superset of standardOffsetTransitionList but doesn't check that
1) ZoneRules.of() implies that transitionList is a superset of
standardOffsetTransitionList but doesn't check that. Then it's possible to
construct ZoneRules instances that don't work correctly:
@Test
public void zoneRulesTest() {
LocalDateTime transitionDay = LocalDateTime.of(2020, 1, 1, 2, 0);
ZoneOffsetTransition trans = ZoneOffsetTransition.of(
transitionDay,
ZoneOffset.ofHours(1),
ZoneOffset.ofHours(2)
);
ZoneRules rules = ZoneRules.of(ZoneOffset.ofHours(1),
ZoneOffset.ofHours(1),
Arrays.asList(trans),
Collections.emptyList(), Collections.emptyList());
Assert.assertEquals(ZoneOffset.ofHours(2), rules.getOffset(
transitionDay.plusDays(7).toInstant(ZoneOffset.UTC)));
}
2) Unrelated issue in java-time code: in ZoneOffsetTransitionRule, there is
a typo, some variables are called "timeDefnition" (missed "i") and it leaks
to public Javadoc.
Re: RFR: 8233166: jpackage tool skips empty directories
looks good. /Andy On 1/29/2020 4:02 PM, Alexander Matveev wrote: Please review the jpackage fix for bug [1] at [2]. - Fixed code which enumerates folders and files to include empty folders. - Added EmptyFolderTest. [1] https://bugs.openjdk.java.net/browse/JDK-8233166 [2] http://cr.openjdk.java.net/~almatvee/8233166/webrev.00/ Thanks, Alexander
Re: RFR: 8233166: jpackage tool skips empty directories
I guess we need tests for empty folders in packages on all platforms. They can be a part of the fix or a follow up CR. Whatever your prefer. - Alexey On 1/29/2020 4:02 PM, Alexander Matveev wrote: Please review the jpackage fix for bug [1] at [2]. - Fixed code which enumerates folders and files to include empty folders. - Added EmptyFolderTest. [1] https://bugs.openjdk.java.net/browse/JDK-8233166 [2] http://cr.openjdk.java.net/~almatvee/8233166/webrev.00/ Thanks, Alexander
RFR: 8233166: jpackage tool skips empty directories
Please review the jpackage fix for bug [1] at [2]. - Fixed code which enumerates folders and files to include empty folders. - Added EmptyFolderTest. [1] https://bugs.openjdk.java.net/browse/JDK-8233166 [2] http://cr.openjdk.java.net/~almatvee/8233166/webrev.00/ Thanks, Alexander
Re: RFR: JDK-8238168: Remove Copyright from WinLauncher.template
+1 -phil. On 1/29/20, 10:34 AM, Andy Herrick wrote: Please review trivial jpackage fix to [1] at [2] [1] https://bugs.openjdk.java.net/browse/JDK-8238168 [2] http://cr.openjdk.java.net/~herrick/8238168/webrev.01/ /Andy
Re: RFR: JDK-8238168: Remove Copyright from WinLauncher.template
Looks good. - Alexey On 1/29/2020 1:34 PM, Andy Herrick wrote: Please review trivial jpackage fix to [1] at [2] [1] https://bugs.openjdk.java.net/browse/JDK-8238168 [2] http://cr.openjdk.java.net/~herrick/8238168/webrev.01/ /Andy
Re: RFR: JDK-8238168: Remove Copyright from WinLauncher.template
Looks good. +1 -- Kevin On 1/29/2020 10:34 AM, Andy Herrick wrote: Please review trivial jpackage fix to [1] at [2] [1] https://bugs.openjdk.java.net/browse/JDK-8238168 [2] http://cr.openjdk.java.net/~herrick/8238168/webrev.01/ /Andy
RFR: JDK-8238168: Remove Copyright from WinLauncher.template
Please review trivial jpackage fix to [1] at [2] [1] https://bugs.openjdk.java.net/browse/JDK-8238168 [2] http://cr.openjdk.java.net/~herrick/8238168/webrev.01/ /Andy
Re: RFR: 8237521: Memory Access API fixes for 32-bit
On 1/29/20 8:32 AM, Maurizio Cimadamore wrote: On 28/01/2020 23:31, David Holmes wrote: I tested again with jdk_core and hotspot_all_no_apps with no new failures. Would you or Maurizio mind sponsoring this for me if your testing is OK? Sorry, as the bulk of this change is core-libs code you'll need to find a core-libs sponsor. Since I'm the one who wrote the core-libs part of the memory access API - I think that should be enough :-) ? Yes, of course. This patch is specific to the memory access API and unsafe. You can also count me as the reviewer. I'm happy to sponsor, until somebody has objections. No objection. Thank you for doing that. Mandy
Re: RFR: 8223260: NamingManager should cache InitialContextFactory
Thanks for the reviews. I found an issue with the new test also - it's loading the custom factory class via the non-serviceloader approach. I was hoping to exercise ServiceLoader here. I'll address this and the comments raised and revert with a new patch shortly. Regards, Sean. On 29/01/20 16:27, Alan Bateman wrote: On 29/01/2020 15:55, Daniel Fuchs wrote: Hi Seán, http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/ A WeakHashKey with the TCCL as the key should be okay here. If the TCCL is the key then there are good chances that the concrete factory class is expected to be loaded by the TCCL. If that happens then the value will reference the key and nothing will ever get garbage collected. I don't know how much JNDI is used much beyond LDAP these days but you are right that a factory with a strong ref to the TCCL would prevent it from being GC'ed. The internal WeakPairMap might be useful here. -Alan
Re: RFR: 8237521: Memory Access API fixes for 32-bit
On 28/01/2020 23:31, David Holmes wrote: Hi Nick, On 27/01/2020 4:41 pm, Nick Gasson wrote: Hi David, On 01/25/20 06:34 am, David Holmes wrote: I've done this here: http://cr.openjdk.java.net/~ngasson/8237521/webrev.02/ Need to check bytes >= 0 before aligning up so that allocateMemory(-1) still throws an IllegalArgumentException. That looks good to me. Tested with jdk_foreign and runtime/Unsafe/. Probably needs a bit more testing. IIRC the direct buffers use this API so they would be a good test subject. Or perhaps Maurizio can run this through our CI tier1-3 before pushing? I tested again with jdk_core and hotspot_all_no_apps with no new failures. Would you or Maurizio mind sponsoring this for me if your testing is OK? Sorry, as the bulk of this change is core-libs code you'll need to find a core-libs sponsor. Since I'm the one who wrote the core-libs part of the memory access API - I think that should be enough :-) ? I'm happy to sponsor, until somebody has objections. Maurizio David Thanks, Nick
Re: RFR: 8223260: NamingManager should cache InitialContextFactory
On 29/01/2020 15:55, Daniel Fuchs wrote: Hi Seán, http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/ A WeakHashKey with the TCCL as the key should be okay here. If the TCCL is the key then there are good chances that the concrete factory class is expected to be loaded by the TCCL. If that happens then the value will reference the key and nothing will ever get garbage collected. I don't know how much JNDI is used much beyond LDAP these days but you are right that a factory with a strong ref to the TCCL would prevent it from being GC'ed. The internal WeakPairMap might be useful here. -Alan
Re: RFR: 8223260: NamingManager should cache InitialContextFactory
Hi Seán, http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/ A WeakHashKey with the TCCL as the key should be okay here. If the TCCL is the key then there are good chances that the concrete factory class is expected to be loaded by the TCCL. If that happens then the value will reference the key and nothing will ever get garbage collected. So if I'm reading things rights this will not work as expected? best regards, -- daniel
