Re: [15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters
+1 -Joe On 4/29/2020 3:18 PM, naoto.s...@oracle.com wrote: Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244152 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8244152/webrev.00/ The hash map used there didn't have initial capacity, even though the exact numbers are known. Naoto
Re: RFR[8183266] - [TESTBUG]Add test to cover XPathEvaluationResult.XPathResultType.getQNameType method
Hi Fernando, Thanks for adding coverage to this API. The change looks good overall. A couple of comments to the test: Line 39 instead of LocalPart, it may be better to verify the QName themselves, I mean, assert the QNames are equal. Line 19-21: the comment block can be moved to a @summary tag with a message sth. like "Verifies the specification of the XPathEvaluationResult API" to allow potential future additions. Best, Joe On 4/29/2020 4:50 AM, Fernando Guallini wrote: Hi all, Please, review the following change: webrev: http://cr.openjdk.java.net/~joehw/jdk15/8183266/webrev/ bug: https://bugs.openjdk.java.net/browse/JDK-8183266 Change details: - Added test coverage for XPathEvaluationResult.XPathResultType.getQNameType method - Added type check for the getQNameType flow restricting the Number class subtypes to satisfy the spec (Integer, Double and Long) - Updated equalsClassType method to be null safe. Kind regards, Fernando
Re: [15] RFR: 8243664: JavaDoc of CompactNumberFormat points to wrong enum
+1.
Indeed, the resulting javadoc was fine :-)
Best,
Joe
On 4/27/2020 9:36 AM, naoto.s...@oracle.com wrote:
Hello,
Please review this small doc fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8243664
Here is the diff:
---
--- old/src/java.base/share/classes/java/text/CompactNumberFormat.java
2020-04-27 09:09:14.0 -0700
+++ new/src/java.base/share/classes/java/text/CompactNumberFormat.java
2020-04-27 09:09:14.0 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -206,7 +206,7 @@
* {@link java.math.RoundingMode} for formatting. By default, it uses
* {@link java.math.RoundingMode#HALF_EVEN RoundingMode.HALF_EVEN}.
*
- * @see CompactNumberFormat.Style
+ * @see NumberFormat.Style
* @see NumberFormat
* @see DecimalFormat
* @since 12
---
javadoc is clever enough to convert it to correct NumberFormat.Style
link, but still this should be corrected.
Naoto
RFR [15/java.xml] 8242470 : Upgrade Xerces to Version 2.12.1
Please review an update to Xerces 2.12.1. Xerces 2.12.1 was a bug fix release. Bugs in the release were either already in the JDK or not applicable. This patch therefore includes only an update to the md file. --- a/src/java.xml/share/legal/xerces.md +++ b/src/java.xml/share/legal/xerces.md @@ -1,4 +1,4 @@ -## Apache Xerces v2.12.0 +## Apache Xerces v2.12.1 ### Apache Xerces Notice @@ -8,9 +8,11 @@ = Apache Xerces Java - Copyright 1999-2018 The Apache Software Foundation + Copyright 1999-2020 The Apache Software Foundation + This product includes software developed at The Apache Software Foundation (http://www.apache.org/). + Portions of this software were originally based on the following: - software copyright (c) 1999, IBM Corporation., http://www.ibm.com. - software copyright (c) 1999, Sun Microsystems., http://www.sun.com. Thanks, Joe
RFR [15/java.xml] 8237187 Obsolete references to java.sun.com
Please review a doc-only change to replace links to sun.com with ones to oracle.com. Only material changes were the two sun.com links, others format, e.g. moving the anchor brackets to within one line. --- a/src/java.base/share/classes/jdk/internal/util/xml/impl/ParserSAX.java +++ b/src/java.base/share/classes/jdk/internal/util/xml/impl/ParserSAX.java @@ -40,14 +40,14 @@ /** * XML non-validating push parser. - * - * This non-validating parser conforms to href="http://www.w3.org/TR/REC-xml; - * >Extensible Markup Language (XML) 1.0 and http://www.w3.org/TR/REC-xml-names; >"Namespaces in XML" - * specifications. The API supported by the parser are - * href="http://java.sun.com/aboutJava/communityprocess/final/jsr030/index.html;>CLDC - * 1.0 and href="http://www.jcp.org/en/jsr/detail?id=280;>JSR-280, a - * JavaME subset of href="http://java.sun.com/xml/jaxp/index.html;>JAXP + * + * This non-validating parser conforms to href="http://www.w3.org/TR/REC-xml;> + * Extensible Markup Language (XML) 1.0 and + * http://www.w3.org/TR/REC-xml-names; >Namespaces in XML + * specifications. The API supported by the parser are + * href="https://www.oracle.com/technetwork/java/cldc-141990.html;>CLDC and + * http://www.jcp.org/en/jsr/detail?id=280;>JSR-280, a JavaME subset of + * href="https://www.oracle.com/technetwork/java/intro-140052.html;>JAXP * and http://www.saxproject.org/;>SAX2. * * @see org.xml.sax.XMLReader Thanks, Joe
Re: [15] RFR: 8242010: Upgrade IANA Language Subtag Registry to Version 2020-04-01
+1 Best, Joe On 4/7/20 12:48 PM, naoto.s...@oracle.com wrote: Thanks Roger. I will fix the typo before the push. Naoto On 4/7/20 12:47 PM, Roger Riggs wrote: Hi Naoto, Looks fine. In EquivMapsGenerator.java. typo: grandfahered -> grandfathered Thanks, Roger On 4/7/20 1:52 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8242010 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8242010/webrev.00/ Besides the upgrade of the data file, I fixed the issue where EquivMapsGenerator had been ignoring "extlang" tag. Now it correctly maps the extlang, such as "zh-cmn" to its preferred language (in this case, "cmn") tag. Naoto
Re: RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element
On 3/30/20 3:12 PM, naoto.s...@oracle.com wrote: Hi Joe, Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null. Indeed, null check is removed: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_03/ Thanks, Joe Naoto On 3/30/20 2:56 PM, Joe Wang wrote: Hi Naoto, Thanks for the review! I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711). Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/ -Joe On 3/30/20 11:23 AM, naoto.s...@oracle.com wrote: Hi Joe, Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version. Otherwise looks good to me. Naoto On 3/30/20 11:02 AM, Joe Wang wrote: Hi, Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711). JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/ Thanks, Joe
Re: RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element
Hi Naoto, Thanks for the review! I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711). Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/ -Joe On 3/30/20 11:23 AM, naoto.s...@oracle.com wrote: Hi Joe, Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version. Otherwise looks good to me. Naoto On 3/30/20 11:02 AM, Joe Wang wrote: Hi, Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711). JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/ Thanks, Joe
RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element
Hi, Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711). JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/ Thanks, Joe
Re: [15] RFR: 8241082: Upgrade IANA Language Subtag Registry data to 03-16-2020 version
Hi Naoto, Looks good to me. -Joe On 3/17/20 1:58 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8241082 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8241082/webrev.00/ It is simply updating the data file. Since there is no change in equivalency of language tags, no code change and test change was needed except for the data release date. Instead, I modified the equiv-map source code generator to explicitly specify the initial capacity for hash maps. Naoto
Re: [15] RFR: 8240626: Some of the java.time.chrono.Eras return empty display name for some styles and locales
Hi Naoto, I see. That makes sense. The change looks good to me. Best, Joe On 3/13/20 7:16 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. Since those names are filled at the JDK build time, there is no way to confirm the localized ones are from the locale itself or its parents, unless parsing CLDR's source XML files in the test at the runtime. I think it is enough to just ensure there's no empty names returned at the runtime, IMO. Naoto On 3/13/20 5:00 PM, Joe Wang wrote: Hi Naoto, The existing tests verifies that a display name matches an expected value. I wonder if you'd want to do a bit more than the Boolean assertion with a similar approach as the existing test, that is, check that the fallback values/alias names match expected names. Best, Joe On 3/13/20 1:25 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8240626 The proposed chageset is located at: https://cr.openjdk.java.net/~naoto/8240626/webrev.00/ In some locales, CLDR only provides partial translations of era names, e.g., only HEISEI and REIWA are provided for Japanese Calendar in Greek locale. CLDRConverter needs to supplement those missing translations from parent locales. Naoto
Re: [15] RFR: 8240626: Some of the java.time.chrono.Eras return empty display name for some styles and locales
Hi Naoto, The existing tests verifies that a display name matches an expected value. I wonder if you'd want to do a bit more than the Boolean assertion with a similar approach as the existing test, that is, check that the fallback values/alias names match expected names. Best, Joe On 3/13/20 1:25 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8240626 The proposed chageset is located at: https://cr.openjdk.java.net/~naoto/8240626/webrev.00/ In some locales, CLDR only provides partial translations of era names, e.g., only HEISEI and REIWA are provided for Japanese Calendar in Greek locale. CLDRConverter needs to supplement those missing translations from parent locales. Naoto
Re: RFR [15/java.xml] 8240982 Incorrect copyright header in BCEL 6.4.1 sources
Thanks Naoto, Lance! -Joe On 3/13/20 10:42 AM, Lance Andersen wrote: +1 On Mar 13, 2020, at 1:32 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a quick fix for the missing commas after the 2nd year in the copyright header: diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java --- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java +++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved. */ /* * Licensed to the Apache Software Foundation (ASF) under one or more diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java --- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java +++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved. */ /* * Licensed to the Apache Software Foundation (ASF) under one or more diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java --- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java +++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved. */ /* * Licensed to the Apache Software Foundation (ASF) under one or more Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [15/java.xml] 8240982 Incorrect copyright header in BCEL 6.4.1 sources
Please review a quick fix for the missing commas after the 2nd year in the copyright header: diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java --- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java +++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved. */ /* * Licensed to the Apache Software Foundation (ASF) under one or more diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java --- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java +++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/ExceptionConst.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved. */ /* * Licensed to the Apache Software Foundation (ASF) under one or more diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java --- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java +++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantLong.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved. */ /* * Licensed to the Apache Software Foundation (ASF) under one or more Thanks, Joe
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
The changes look good to me.
Best,
Joe
On 3/9/20 1:44 AM, Stephen Colebourne wrote:
Fine by me, but I'm not an OpenJDK Reviewer
Stephen
On Mon, 9 Mar 2020 at 03:05, wrote:
Thanks, Stephen.
Updated the webrev for those two suggestions:
http://cr.openjdk.java.net/~naoto/8239836/webrev.04/
Naoto
On 3/8/20 4:22 PM, Stephen Colebourne wrote:
standardOffsets[0] == wallOffsets[0] needs to use .equals()
Unrelated, there is a Javadoc/spec error at line 578 of the "new"
file. It should be getValidOffsets, not getOffset.
Apart from that, it looks good.
thanks
Stephen
On Wed, 4 Mar 2020 at 19:06, wrote:
Hi Stephen,
Thank you for the detailed explanation and correcting the
implementation. I incorporated all the suggestions below, as well as
adding a new test for isFixedOffset() implementation (with some clean-up):
https://cr.openjdk.java.net/~naoto/8239836/webrev.03/
Please take a look at it.
Naoto
On 3/3/20 3:30 PM, Stephen Colebourne wrote:
I fear more changes are needed here.
1) The code is isFixedOffset() is indeed wrong, but the fix is
insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
and all three other lists are empty. A fixed offset rule is the
equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
assumes that.
2) The code in getOffset(Instant) is wrong, but so is the proposed
fix. The purpose of the if statement is to optimise the following few
lines which refer to savingsInstantTransitions and wallOffsets. The
code does have a bug as it should return the first wall offset.
Corrected code:
public ZoneOffset getOffset(Instant instant) {
if (savingsInstantTransitions.length == 0) {
return wallOffsets[0];
}
With the correct definition of isFixedOffset() it becomes apparent
that this if statement is in fact not related to the fixed offset.
Currently this test case fails (a zone in DST for all eternity):
ZoneRules rules = ZoneRules.of(
ZoneOffset.ofHours(1),
ZoneOffset.ofHours(2),
Collections.emptyList(),
Collections.emptyList(),
Collections.emptyList());
assertEquals(rules.getStandardOffset(Instant.EPOCH),
ZoneOffset.ofHours(1));
assertEquals(rules.getOffset(Instant.EPOCH), ZoneOffset.ofHours(2));
3) The code in getStandardOffset(Instant) also has an error as it
checks the wrong array. It should check standardTransitions as that is
what is used in the following few lines. Corrected code:
public ZoneOffset getStandardOffset(Instant instant) {
if (standardTransitions.length == 0) {
return standardOffsets[0];
}
4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
Since it protects savingsLocalTransitions, it should return
wallOffsets[0].
5) The code in getDaylightSavings(Instant instant) has an optimising
if statement that should refer to isFixedOffset().
6) Also, in the test.
assertTrue(!rules.isFixedOffset());
should be
assertFalse(rules.isFixedOffset());
The class has worked so far as every normal case has
baseStandardOffset = baseWallOffset, even though it is conceptually
valid for this not to be true.
Stephen
On Thu, 27 Feb 2020 at 20:42, wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8239836
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
It turned out that 'transitionList' is not necessarily a superset of
'standardOffsetTransitionList', as in some occasions where a standard
offset change and a savings change happen at the same time (canceling
each other), resulting in no wall time transition. This means that the
"rules" in the sample code is a valid ZoneRules instance.
However, there is an assumption in ZoneRules where no (wall time)
transition means the fixed offset zone. With that assumption, the
example rule is considered a fixed offset zone which is not correct. So
the fix proposed here is not to throw an IAE but to recognize the rule
as a valid, non-fixed offset ZoneRules instance.
Naoto
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Looks good to me.
Thanks,
Joe
On 3/3/20 10:15 AM, naoto.s...@oracle.com wrote:
Thanks, Joe. Updated:
http://cr.openjdk.java.net/~naoto/8239836/webrev.02/
Naoto
On 3/3/20 8:59 AM, Joe Wang wrote:
On 3/3/20 8:27 AM, naoto.s...@oracle.com wrote:
Hi Joe, thanks for the review.
Are you suggesting something like
isFixedOffset() {
return isFixedOffset;
}
Yes, something like that. It could be initiated while the rules is
constructed. I feel it might be semantically clearer as transitions
indirectly reflect that the offset is fixed. Your call.
Best,
Joe
Naoto
On 3/2/20 11:20 PM, Joe Wang wrote:
Hi Naoto,
The fix would be fine if you want to keep it as is since it does work.
I noticed though that for standard zones (the ones loaded from tz
database), savingsInstantTransitions and standardTransitions are
consistent in that they are both empty for the standard zones, e.g.
Etc/GMT, and not empty for zones with a country/city id, including
countries that don't actually observe DST. This means a check for
one of them is enough for standard zones, which was done in the
current implementation (that is, isFixedOffset() returns
savingsInstantTransitions.length == 0). For custom ZoneRules
created with the "of" method, it would depend on whether they are
set through the relevant parameters (in the test case, the later
was set but the former was empty, that was why isFixedOffset was
true). It would therefore be possible to add and use a transient
boolean field to represent isFixedOffset.
Just my two cents.
-Joe
On 3/2/20 10:37 AM, Roger Riggs wrote:
Looks good.
Give it a day to see if anyone else has comments.
Thanks, Roger
On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote:
Hi Roger, thanks for the review.
On 3/2/20 8:44 AM, Roger Riggs wrote:
Hi Naoto,
look ok.
ZoneRules.java: 488, 644, 761, 791
I'd suggest calling isFixedOffset() instead of repeating the code:
standardTransitions.length == 0 &&
savingsInstantTransitions.length == 0
Modified as suggested:
http://cr.openjdk.java.net/~naoto/8239836/webrev.01/
It should be inlined in cases where the performance matters.
None of those locations is invoked during ZoneRules object
instantiation. So I believe it is OK to replace them with
isFixedOffset().
Naoto
Thanks, Roger
On 2/27/20 3:41 PM, naoto.s...@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8239836
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
It turned out that 'transitionList' is not necessarily a
superset of
'standardOffsetTransitionList', as in some occasions where a
standard offset change and a savings change happen at the same
time (canceling each other), resulting in no wall time
transition. This means that the "rules" in the sample code is a
valid ZoneRules instance.
However, there is an assumption in ZoneRules where no (wall
time) transition means the fixed offset zone. With that
assumption, the example rule is considered a fixed offset zone
which is not correct. So the fix proposed here is not to throw
an IAE but to recognize the rule as a valid, non-fixed offset
ZoneRules instance.
Naoto
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
On 3/3/20 8:27 AM, naoto.s...@oracle.com wrote:
Hi Joe, thanks for the review.
Are you suggesting something like
isFixedOffset() {
return isFixedOffset;
}
Yes, something like that. It could be initiated while the rules is
constructed. I feel it might be semantically clearer as transitions
indirectly reflect that the offset is fixed. Your call.
Best,
Joe
Naoto
On 3/2/20 11:20 PM, Joe Wang wrote:
Hi Naoto,
The fix would be fine if you want to keep it as is since it does work.
I noticed though that for standard zones (the ones loaded from tz
database), savingsInstantTransitions and standardTransitions are
consistent in that they are both empty for the standard zones, e.g.
Etc/GMT, and not empty for zones with a country/city id, including
countries that don't actually observe DST. This means a check for one
of them is enough for standard zones, which was done in the current
implementation (that is, isFixedOffset() returns
savingsInstantTransitions.length == 0). For custom ZoneRules created
with the "of" method, it would depend on whether they are set through
the relevant parameters (in the test case, the later was set but the
former was empty, that was why isFixedOffset was true). It would
therefore be possible to add and use a transient boolean field to
represent isFixedOffset.
Just my two cents.
-Joe
On 3/2/20 10:37 AM, Roger Riggs wrote:
Looks good.
Give it a day to see if anyone else has comments.
Thanks, Roger
On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote:
Hi Roger, thanks for the review.
On 3/2/20 8:44 AM, Roger Riggs wrote:
Hi Naoto,
look ok.
ZoneRules.java: 488, 644, 761, 791
I'd suggest calling isFixedOffset() instead of repeating the code:
standardTransitions.length == 0 &&
savingsInstantTransitions.length == 0
Modified as suggested:
http://cr.openjdk.java.net/~naoto/8239836/webrev.01/
It should be inlined in cases where the performance matters.
None of those locations is invoked during ZoneRules object
instantiation. So I believe it is OK to replace them with
isFixedOffset().
Naoto
Thanks, Roger
On 2/27/20 3:41 PM, naoto.s...@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8239836
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
It turned out that 'transitionList' is not necessarily a superset of
'standardOffsetTransitionList', as in some occasions where a
standard offset change and a savings change happen at the same
time (canceling each other), resulting in no wall time
transition. This means that the "rules" in the sample code is a
valid ZoneRules instance.
However, there is an assumption in ZoneRules where no (wall time)
transition means the fixed offset zone. With that assumption, the
example rule is considered a fixed offset zone which is not
correct. So the fix proposed here is not to throw an IAE but to
recognize the rule as a valid, non-fixed offset ZoneRules instance.
Naoto
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Hi Naoto, The fix would be fine if you want to keep it as is since it does work. I noticed though that for standard zones (the ones loaded from tz database), savingsInstantTransitions and standardTransitions are consistent in that they are both empty for the standard zones, e.g. Etc/GMT, and not empty for zones with a country/city id, including countries that don't actually observe DST. This means a check for one of them is enough for standard zones, which was done in the current implementation (that is, isFixedOffset() returns savingsInstantTransitions.length == 0). For custom ZoneRules created with the "of" method, it would depend on whether they are set through the relevant parameters (in the test case, the later was set but the former was empty, that was why isFixedOffset was true). It would therefore be possible to add and use a transient boolean field to represent isFixedOffset. Just my two cents. -Joe On 3/2/20 10:37 AM, Roger Riggs wrote: Looks good. Give it a day to see if anyone else has comments. Thanks, Roger On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote: Hi Roger, thanks for the review. On 3/2/20 8:44 AM, Roger Riggs wrote: Hi Naoto, look ok. ZoneRules.java: 488, 644, 761, 791 I'd suggest calling isFixedOffset() instead of repeating the code: standardTransitions.length == 0 && savingsInstantTransitions.length == 0 Modified as suggested: http://cr.openjdk.java.net/~naoto/8239836/webrev.01/ It should be inlined in cases where the performance matters. None of those locations is invoked during ZoneRules object instantiation. So I believe it is OK to replace them with isFixedOffset(). Naoto Thanks, Roger On 2/27/20 3:41 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8239836 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8239836/webrev.00/ It turned out that 'transitionList' is not necessarily a superset of 'standardOffsetTransitionList', as in some occasions where a standard offset change and a savings change happen at the same time (canceling each other), resulting in no wall time transition. This means that the "rules" in the sample code is a valid ZoneRules instance. However, there is an assumption in ZoneRules where no (wall time) transition means the fixed offset zone. With that assumption, the example rule is considered a fixed offset zone which is not correct. So the fix proposed here is not to throw an IAE but to recognize the rule as a valid, non-fixed offset ZoneRules instance. Naoto
Re: RFR(S) : 8238943: switch to jtreg 5.0
+1 for the change to test/jaxp/TEST.ROOT. Best, Joe On 2/13/20 10:08 AM, Igor Ignatev wrote: Oh, I’m sorry I actually changed it to 5.0 when were (re)doing testing, and apparently forgot to replace the webrev, the right is http://cr.openjdk.java.net/~iignatyev//8238943/webrev.01 ; with version field value being the only difference b/w .00 and .01 Thanks, — Igor On Feb 13, 2020, at 9:23 AM, Erik Joelsson wrote: Looks good, but could you change the "version" field to "5.0", it should work now. /Erik On 2020-02-13 08:50, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00 10 lines changed: 1 ins; 0 del; 9 mod; Hi all, could you please review the patch which changes jtreg version used in jdk/jdk to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this patch also bumps requiredVersion in all the jtreg test suites in order to reduce possible discrepancy in results. JBS: https://bugs.openjdk.java.net/browse/JDK-8238943 webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00 testing: tier1-4 Thanks, -- Igor
Re: [15] RFR: 8234347: "Turkey" meta time zone does not generate composed localized names, 8236548: Localized time zone name inconsistency between English and other locales
+1. That's nicer. -Joe On 2/11/20 10:17 AM, naoto.s...@oracle.com wrote: Hi, I modified the proposed changeset. Removed the hard coded 3-letter id compatibility mappings (oldMappings) from CLDRConverter.java. Instead, using public ZoneId.SHORT_IDS that contain the same set of mappings (wasn't aware it was defined in the spec!) Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8234347.8236548/webrev.01/ Naoto On 2/7/20 1:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issues: https://bugs.openjdk.java.net/browse/JDK-8234347 https://bugs.openjdk.java.net/browse/JDK-8236548 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8234347.8236548/webrev.00/ This changeset includes the following changes: - English time zone names missing in CLDR source files are no longer copied from COMPAT provider at build time. Rather it is synthesized at runtime, which has been the way other locales did. - Runtime CLDR time zone name provider fallback logic has been refined. It now falls back to parent locales per each missing name, instead of resource bundle. Also, region fall back is now using exemplar city to synthesize the name (e.g., "Turkey" meta zone) - Minor fix in DateTimeFormatterBuilder on zone text parsing. It now parses zone texts that start with "UTC", yet it is ZoneId. Naoto
Re: [15] RFR: 8234347: "Turkey" meta time zone does not generate composed localized names, 8236548: Localized time zone name inconsistency between English and other locales
Hi Naoto, I see the existing tests were changed, e.g. the abbreviation / short timezone name, the result of calling getDisplayName. Would you need a CSR to discuss/document the potential incompatibility? Best, Joe On 2/7/20 1:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issues: https://bugs.openjdk.java.net/browse/JDK-8234347 https://bugs.openjdk.java.net/browse/JDK-8236548 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8234347.8236548/webrev.00/ This changeset includes the following changes: - English time zone names missing in CLDR source files are no longer copied from COMPAT provider at build time. Rather it is synthesized at runtime, which has been the way other locales did. - Runtime CLDR time zone name provider fallback logic has been refined. It now falls back to parent locales per each missing name, instead of resource bundle. Also, region fall back is now using exemplar city to synthesize the name (e.g., "Turkey" meta zone) - Minor fix in DateTimeFormatterBuilder on zone text parsing. It now parses zone texts that start with "UTC", yet it is ZoneId. Naoto
Re: [14] RFR (XXS) 8238605: Correct the CLDR version number in cldr.md files
+1 Best, Joe On 2/6/20 8:50 AM, naoto.s...@oracle.com wrote: Hello, Please review this extra small fix for this issue: https://bugs.openjdk.java.net/browse/JDK-8238605 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8238605/webrev.00/ It is simply updating the version number in cldr.md files, which should have been done with JDK-8231273. Naoto
Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
Thanks Daniel! -Joe On 1/17/20 3:57 AM, Daniel Fuchs wrote: On 16/01/2020 18:22, Joe Wang wrote: It's because the class itself is declared final (at least on the few files I've taken a look), so final on a method is redundant. Ah! I had missed that. Thaks Rémi! Meanwhile, I noticed I missed the new classes in the webrev. I used a changelist to create webrevs and forgot to add the new ones to the list. Sorry about that. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev_02/ LGTM. best regards, -- daniel
Re: RFR: 8237465: JDK 14 L10N resource files update - msg drop 10
On 1/17/20 10:31 AM, naoto.s...@oracle.com wrote: Hi Leo, The l10n files for HelpResources_*.properties in jpackage discard the format in the original English resource file where lines are nicely aligned. Can you please preserve the format? Also, I find it odd to observe duplicated String literals in every XPATHErrorResources_*.java files. I think they should only be in the base resource bundles, but I guess that's in the original xml code and not l10n related. Yes, it's from Apache. -Joe Naoto On 1/17/20 3:31 AM, li.ji...@oracle.com wrote: Hi, Please review the L10N resource files update for JDK 14 msg drop 10. https://bugs.openjdk.java.net/browse/JDK-8237465 http://cr.openjdk.java.net/~ljiang/8237465/webrev/read/ This is the first msg drop for JDK 14 L10N resources files update, so this webrev would cover most of update for JDK 14. We still have the msg drop 20 in the next days, so pls commit the l10n related changes before Jan 23rd. After msg drop 20, we kick off the msg drop 30 only if necessary, showstopper only. Note: some resource changes in preview features, like Record, Pattern Matching, are supported in this msg drop, also some new resource files for jpackager were added. Thanks, Leo
Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
Thanks Lance, again :-) -Joe On 1/16/20 2:39 PM, Lance Andersen wrote: Hi Joe, The additions look OK also Best Lance On Jan 16, 2020, at 1:22 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: On 1/16/20 9:50 AM, Remi Forax wrote: - Mail original ----- De: "Joe Wang" mailto:huizhe.w...@oracle.com>> À: "Daniel Fuchs" <mailto:daniel.fu...@oracle.com>>, "core-libs-dev" <mailto:core-libs-dev@openjdk.java.net>> Envoyé: Jeudi 16 Janvier 2020 18:40:18 Objet: Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1 On 1/16/20 2:35 AM, Daniel Fuchs wrote: Hi Joe, Looks OK to me as well. Thanks for the review! I am a bit surprised by the number of methods that are no longer `final` though. Do you know what was the motivation for those changes? The original patch did not have any detailed comment or link to a bug report. The title was "Remove redundant modifiers. Minor Javadoc and formatting. " So it seemed they were cleaning up "redundant modifiers". It would be interesting if that's the reason to remove 'final'. However, it has no impact on our usage of the library in java.xml. It's because the class itself is declared final (at least on the few files I've taken a look), so final on a method is redundant. Aha, indeed! I double-checked classes such as Code.java, CodeException.java, etc. Meanwhile, I noticed I missed the new classes in the webrev. I used a changelist to create webrevs and forgot to add the new ones to the list. Sorry about that. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev_02/ Thanks, Joe Best regards, Joe regards, Rémi best regards, -- daniel On 14/01/2020 20:08, Joe Wang wrote: Hi, Please review an update to BCEL 6.4.1. JBS: https://bugs.openjdk.java.net/browse/JDK-8235368 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev/index.html Similar approach as the last update: 1. Format All format changes are kept as they are in the source in order to reduce the amount of changes in future updates, the exceptions are extreme long lines. 2. Exclusions Contents that were not in the JDK or unnecessary for java.xml are excluded. This includes: the ability to load arbitrary classes and classes related to ClassLoader, ClassPath and JavaWrapper, and relevant methods and references in other classes; System Properties used to set cache sizes and track certain statistics (caches are set as in previous version); Deprecated classes and related contents. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. They are re-applied for this update. The LastModified field indicates such changes to the original source. 4. Deprecated fields to private and references to deprecated methods Deprecated fields in the original source were changed to private ones in previous update. References to deprecated methods were modified to use proper methods. These changes are inherited in this update. 5. Test Since the update does not affect java.xml's usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. A performance test is running. Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
On 1/16/20 9:50 AM, Remi Forax wrote: - Mail original - De: "Joe Wang" À: "Daniel Fuchs" , "core-libs-dev" Envoyé: Jeudi 16 Janvier 2020 18:40:18 Objet: Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1 On 1/16/20 2:35 AM, Daniel Fuchs wrote: Hi Joe, Looks OK to me as well. Thanks for the review! I am a bit surprised by the number of methods that are no longer `final` though. Do you know what was the motivation for those changes? The original patch did not have any detailed comment or link to a bug report. The title was "Remove redundant modifiers. Minor Javadoc and formatting. " So it seemed they were cleaning up "redundant modifiers". It would be interesting if that's the reason to remove 'final'. However, it has no impact on our usage of the library in java.xml. It's because the class itself is declared final (at least on the few files I've taken a look), so final on a method is redundant. Aha, indeed! I double-checked classes such as Code.java, CodeException.java, etc. Meanwhile, I noticed I missed the new classes in the webrev. I used a changelist to create webrevs and forgot to add the new ones to the list. Sorry about that. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev_02/ Thanks, Joe Best regards, Joe regards, Rémi best regards, -- daniel On 14/01/2020 20:08, Joe Wang wrote: Hi, Please review an update to BCEL 6.4.1. JBS: https://bugs.openjdk.java.net/browse/JDK-8235368 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev/index.html Similar approach as the last update: 1. Format All format changes are kept as they are in the source in order to reduce the amount of changes in future updates, the exceptions are extreme long lines. 2. Exclusions Contents that were not in the JDK or unnecessary for java.xml are excluded. This includes: the ability to load arbitrary classes and classes related to ClassLoader, ClassPath and JavaWrapper, and relevant methods and references in other classes; System Properties used to set cache sizes and track certain statistics (caches are set as in previous version); Deprecated classes and related contents. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. They are re-applied for this update. The LastModified field indicates such changes to the original source. 4. Deprecated fields to private and references to deprecated methods Deprecated fields in the original source were changed to private ones in previous update. References to deprecated methods were modified to use proper methods. These changes are inherited in this update. 5. Test Since the update does not affect java.xml's usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. A performance test is running. Thanks, Joe
Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
On 1/16/20 2:35 AM, Daniel Fuchs wrote: Hi Joe, Looks OK to me as well. Thanks for the review! I am a bit surprised by the number of methods that are no longer `final` though. Do you know what was the motivation for those changes? The original patch did not have any detailed comment or link to a bug report. The title was "Remove redundant modifiers. Minor Javadoc and formatting. " So it seemed they were cleaning up "redundant modifiers". It would be interesting if that's the reason to remove 'final'. However, it has no impact on our usage of the library in java.xml. Best regards, Joe best regards, -- daniel On 14/01/2020 20:08, Joe Wang wrote: Hi, Please review an update to BCEL 6.4.1. JBS: https://bugs.openjdk.java.net/browse/JDK-8235368 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev/index.html Similar approach as the last update: 1. Format All format changes are kept as they are in the source in order to reduce the amount of changes in future updates, the exceptions are extreme long lines. 2. Exclusions Contents that were not in the JDK or unnecessary for java.xml are excluded. This includes: the ability to load arbitrary classes and classes related to ClassLoader, ClassPath and JavaWrapper, and relevant methods and references in other classes; System Properties used to set cache sizes and track certain statistics (caches are set as in previous version); Deprecated classes and related contents. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. They are re-applied for this update. The LastModified field indicates such changes to the original source. 4. Deprecated fields to private and references to deprecated methods Deprecated fields in the original source were changed to private ones in previous update. References to deprecated methods were modified to use proper methods. These changes are inherited in this update. 5. Test Since the update does not affect java.xml's usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. A performance test is running. Thanks, Joe
Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
Thanks Lance! -Joe On 1/15/20 2:21 PM, Lance Andersen wrote: Hi Joe, This seems OK. On Jan 14, 2020, at 3:08 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Hi, Please review an update to BCEL 6.4.1. JBS: https://bugs.openjdk.java.net/browse/JDK-8235368 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev/index.html Similar approach as the last update: 1. Format All format changes are kept as they are in the source in order to reduce the amount of changes in future updates, the exceptions are extreme long lines. 2. Exclusions Contents that were not in the JDK or unnecessary for java.xml are excluded. This includes: the ability to load arbitrary classes and classes related to ClassLoader, ClassPath and JavaWrapper, and relevant methods and references in other classes; System Properties used to set cache sizes and track certain statistics (caches are set as in previous version); Deprecated classes and related contents. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. They are re-applied for this update. The LastModified field indicates such changes to the original source. 4. Deprecated fields to private and references to deprecated methods Deprecated fields in the original source were changed to private ones in previous update. References to deprecated methods were modified to use proper methods. These changes are inherited in this update. 5. Test Since the update does not affect java.xml's usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. A performance test is running. Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology
On 1/14/20 6:04 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. Please find my comments below: On 1/14/20 3:35 PM, Joe Wang wrote: Hi Naoto, Since it's dealing with non-standard properties files, is there a need to verify the files? The constructor (HijrahChronology) does check whether the id or type is empty. If there is no existing process to validate, it's probably not worth it to spend time as it's rare and it's config time. IIUC, the idea is to create the instance at class loading time (thus the faster the better) and cache it, then check the validity later at actual method invocation (cf. checkCalendarInit() method). Make sense. The test summary states "Test image creation", it may be better to say sth. like verifies custom configuration? Good catch! I was simply copying some portion from other test case. Corrected: https://cr.openjdk.java.net/~naoto/8187987/webrev.01/ Looks good to me. Best regards, Joe Naoto Best, Joe On 1/14/20 8:35 AM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8187987 The proposed CSR and changeset are located at: CSR: https://bugs.openjdk.java.net/browse/JDK-8236810 Webrev: https://cr.openjdk.java.net/~naoto/8187987/webrev.00/ The spec of java.time.chrono.HijrahChronology suggests that it could load custom variants of Hijirah calendar type using properties files. However it does not work as designed. This change intends to make it work. Naoto
Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology
Hi Naoto, Since it's dealing with non-standard properties files, is there a need to verify the files? The constructor (HijrahChronology) does check whether the id or type is empty. If there is no existing process to validate, it's probably not worth it to spend time as it's rare and it's config time. The test summary states "Test image creation", it may be better to say sth. like verifies custom configuration? Best, Joe On 1/14/20 8:35 AM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8187987 The proposed CSR and changeset are located at: CSR: https://bugs.openjdk.java.net/browse/JDK-8236810 Webrev: https://cr.openjdk.java.net/~naoto/8187987/webrev.00/ The spec of java.time.chrono.HijrahChronology suggests that it could load custom variants of Hijirah calendar type using properties files. However it does not work as designed. This change intends to make it work. Naoto
Re: RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
Performance test results show no regression over the current build (15-b5). -Joe On 1/14/20 12:08 PM, Joe Wang wrote: A performance test is running.
RFR [15/java.xml] 8235368 : Update BCEL to Version 6.4.1
Hi, Please review an update to BCEL 6.4.1. JBS: https://bugs.openjdk.java.net/browse/JDK-8235368 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8235368/webrev/index.html Similar approach as the last update: 1. Format All format changes are kept as they are in the source in order to reduce the amount of changes in future updates, the exceptions are extreme long lines. 2. Exclusions Contents that were not in the JDK or unnecessary for java.xml are excluded. This includes: the ability to load arbitrary classes and classes related to ClassLoader, ClassPath and JavaWrapper, and relevant methods and references in other classes; System Properties used to set cache sizes and track certain statistics (caches are set as in previous version); Deprecated classes and related contents. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. They are re-applied for this update. The LastModified field indicates such changes to the original source. 4. Deprecated fields to private and references to deprecated methods Deprecated fields in the original source were changed to private ones in previous update. References to deprecated methods were modified to use proper methods. These changes are inherited in this update. 5. Test Since the update does not affect java.xml's usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. A performance test is running. Thanks, Joe
Re: [15] RFR: 8174270: Consolidate ICU sources in one location
I see. It's all good to me then. Best, Joe On 1/10/20 4:04 PM, naoto.s...@oracle.com wrote: Hi Joe, That data file seems no longer included in the ICU4J package (as of 64.2), thus I left it as it is. Naoto On 1/10/20 2:57 PM, Joe Wang wrote: Is there a reason why uidna.spp was left out of the move? -Joe On 1/10/20 2:02 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8174270 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8174270/webrev.00/ Although the number of modified files are huge, the change is simply moving the scattered ICU sources in various locations under jdk.internal.icu package, keeping the original ICU's source tree structure. Naoto
Re: [15] RFR: 8174270: Consolidate ICU sources in one location
Is there a reason why uidna.spp was left out of the move? -Joe On 1/10/20 2:02 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8174270 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8174270/webrev.00/ Although the number of modified files are huge, the change is simply moving the scattered ICU sources in various locations under jdk.internal.icu package, keeping the original ICU's source tree structure. Naoto
Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols
Hi Naoto,
Historical indeed, and true, it was not exposed prior to JDK6. Just need
to make sure other classes within the package won't accidentally use its
set method or mark it if it's 'deprecated'. But it's a minor issue. I'm
fine with your changeset as is.
Best,
Joe
On 1/3/20 1:24 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Thanks again for the review. The reason the way it is is all
historical. percent/perMill/minusSign all had public APIs for the
'char' version since inception, and text version APIs were added later
(JDK13). Thus they had to be in sync (both fields are accessible
through API). On the other hand, exponential was private till JDK6,
and at that time I guess the engineer decided only to expose public
access to its text version, i.e., effectively deprecate 'char' version
interface and field. I guess that's why he/she did not bother make
them in sync, IMO. So there seems to be no explicit reason (to be
noted in the source code) for not syncing.
My $.02
Naoto
On 1/3/20 11:40 AM, Joe Wang wrote:
Hi Naoto,
The change looks fine to me as only monetaryGroupingSeparator was
added to equals.
I can't help to note though that, all fields participated in the
equals calculation except exponential. Some of the other fields are
in similar situations (one is public and the other not), e.g. percent
and percentText, perMill and perMillText, minusSign and
minusSignText, and also the currency related fields, but they all are
included. It looks like exponential was never publicly accessible,
but the (1.6) added exponentialSeparator became public. It's probably
not necessary to include all of them in the first place as they are
in sync. that is, changing one would change the other -- and in this
regard, exponential is an exception: setExponentialSymbol won't
change exponential.
I understand this is all historical and it doesn't affect your
changeset. If the reason is known, it won't hurt to add some notes as
the other setXxxText clearly stated the relationship with their
non-Text representation. If not, it's fine to me to not have to spend
the extra time.
Best,
Joe
On 1/3/20 9:23 AM, naoto.s...@oracle.com wrote:
Hi Joe,
I revised the changeset, as the cached hash code in
DecimalFormatSymbols needs to be recalculated when any of the
relevant fields is mutated. Here is the updated webrev:
http://cr.openjdk.java.net/~naoto/8227313/webrev.02/
Naoto
On 1/2/20 2:19 PM, Joe Wang wrote:
Happy New Year, Naoto!
Thanks for the explanation and changes. The changeset looks good to
me.
-Joe
On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Happy new year and thanks for your comments. Please see my replies
below:
On 12/23/19 5:20 PM, Joe Wang wrote:
Hi Naoto,
Is there a need for an APINote to note the relationship between
the new get/setMonetaryGroupingSeparator and
get/setGroupingSeparator methods. The new method did state it
"May be different from {@code grouping separator} in some
locales", but that may be insufficient. For example, does setting
one method affects the other (seems it should since the
monetaryGroupingSeparator may be initialized by the
groupingSeparator as the impl shows)? If yes, how it's affected?
Setting the custom monetary grouping separator will not affect the
existing normal grouping separator. I added the explanation in the
method description.
If no, is there a compatibility concern? In the current impl, the
new get method is used when "isCurrencyFormat" is true while the
new set method doesn't affect the existing 'groupingSeparator'.
For existing applications that have a custom grouping separator
set through setGroupingSeparator, it seems to me the custom
separator won't be used moving onto the new JDK impl.
Good point. Modified the compatibility risk from minimum to low
with the explanation.
A minor comment about the definition statement in the following:
Add the following new serializable field
in|java.text.DecimalFormatSymbols|class:
|/** * The grouping separator used when formatting currency
values. * * @serial * @since 15 */ private char
monetaryGroupingSeparator;|
and that for the new get method:
Gets the character used for thousands separator for
currencies.
While the meanings are clear, they were not as consistent as that
between the existing groupingSeparator and its get method, that is:
Character used for thousands separator.
and then the get method states:
Gets the character used for thousands separator.
But this is minor, your call whether to change it or not.
Consistency is important. I replaced all occurrences of "thousands
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping
separator(s)."
Here are the modified changeset:
http://cr.openjdk.java.net/~naoto/8227313/webrev.01/
as well as the modified CSR at the same URL.
Naoto
Best, and have a great Christmas
Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols
Hi Naoto,
The change looks fine to me as only monetaryGroupingSeparator was added
to equals.
I can't help to note though that, all fields participated in the equals
calculation except exponential. Some of the other fields are in similar
situations (one is public and the other not), e.g. percent and
percentText, perMill and perMillText, minusSign and minusSignText, and
also the currency related fields, but they all are included. It looks
like exponential was never publicly accessible, but the (1.6) added
exponentialSeparator became public. It's probably not necessary to
include all of them in the first place as they are in sync. that is,
changing one would change the other -- and in this regard, exponential
is an exception: setExponentialSymbol won't change exponential.
I understand this is all historical and it doesn't affect your
changeset. If the reason is known, it won't hurt to add some notes as
the other setXxxText clearly stated the relationship with their non-Text
representation. If not, it's fine to me to not have to spend the extra time.
Best,
Joe
On 1/3/20 9:23 AM, naoto.s...@oracle.com wrote:
Hi Joe,
I revised the changeset, as the cached hash code in
DecimalFormatSymbols needs to be recalculated when any of the relevant
fields is mutated. Here is the updated webrev:
http://cr.openjdk.java.net/~naoto/8227313/webrev.02/
Naoto
On 1/2/20 2:19 PM, Joe Wang wrote:
Happy New Year, Naoto!
Thanks for the explanation and changes. The changeset looks good to me.
-Joe
On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Happy new year and thanks for your comments. Please see my replies
below:
On 12/23/19 5:20 PM, Joe Wang wrote:
Hi Naoto,
Is there a need for an APINote to note the relationship between the
new get/setMonetaryGroupingSeparator and get/setGroupingSeparator
methods. The new method did state it "May be different from {@code
grouping separator} in some locales", but that may be insufficient.
For example, does setting one method affects the other (seems it
should since the monetaryGroupingSeparator may be initialized by
the groupingSeparator as the impl shows)? If yes, how it's affected?
Setting the custom monetary grouping separator will not affect the
existing normal grouping separator. I added the explanation in the
method description.
If no, is there a compatibility concern? In the current impl, the
new get method is used when "isCurrencyFormat" is true while the
new set method doesn't affect the existing 'groupingSeparator'. For
existing applications that have a custom grouping separator set
through setGroupingSeparator, it seems to me the custom separator
won't be used moving onto the new JDK impl.
Good point. Modified the compatibility risk from minimum to low with
the explanation.
A minor comment about the definition statement in the following:
Add the following new serializable field
in|java.text.DecimalFormatSymbols|class:
|/** * The grouping separator used when formatting currency values.
* * @serial * @since 15 */ private char monetaryGroupingSeparator;|
and that for the new get method:
Gets the character used for thousands separator for
currencies.
While the meanings are clear, they were not as consistent as that
between the existing groupingSeparator and its get method, that is:
Character used for thousands separator.
and then the get method states:
Gets the character used for thousands separator.
But this is minor, your call whether to change it or not.
Consistency is important. I replaced all occurrences of "thousands
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping
separator(s)."
Here are the modified changeset:
http://cr.openjdk.java.net/~naoto/8227313/webrev.01/
as well as the modified CSR at the same URL.
Naoto
Best, and have a great Christmas! :-)
Joe
On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:
Hi,
Please review the fix for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8227313
The proposed CSR and changeset are located at:
https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/
The change introduces the new monetary grouping separator in
DecimalFormatSymbols, and it is used if a DecimalFormat instance
designates currency formatting where its pattern includes the
currency symbol (U+00A5). The change makes use of the CLDR data
which provides two distinct grouping separators (one for generic,
the other for currency) in some locales. It also addresses the
similar cases for the decimal separator.
Naoto
Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols
Happy New Year, Naoto!
Thanks for the explanation and changes. The changeset looks good to me.
-Joe
On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Happy new year and thanks for your comments. Please see my replies below:
On 12/23/19 5:20 PM, Joe Wang wrote:
Hi Naoto,
Is there a need for an APINote to note the relationship between the
new get/setMonetaryGroupingSeparator and get/setGroupingSeparator
methods. The new method did state it "May be different from {@code
grouping separator} in some locales", but that may be insufficient.
For example, does setting one method affects the other (seems it
should since the monetaryGroupingSeparator may be initialized by the
groupingSeparator as the impl shows)? If yes, how it's affected?
Setting the custom monetary grouping separator will not affect the
existing normal grouping separator. I added the explanation in the
method description.
If no, is there a compatibility concern? In the current impl, the new
get method is used when "isCurrencyFormat" is true while the new set
method doesn't affect the existing 'groupingSeparator'. For existing
applications that have a custom grouping separator set through
setGroupingSeparator, it seems to me the custom separator won't be
used moving onto the new JDK impl.
Good point. Modified the compatibility risk from minimum to low with
the explanation.
A minor comment about the definition statement in the following:
Add the following new serializable field
in|java.text.DecimalFormatSymbols|class:
|/** * The grouping separator used when formatting currency values. *
* @serial * @since 15 */ private char monetaryGroupingSeparator;|
and that for the new get method:
Gets the character used for thousands separator for currencies.
While the meanings are clear, they were not as consistent as that
between the existing groupingSeparator and its get method, that is:
Character used for thousands separator.
and then the get method states:
Gets the character used for thousands separator.
But this is minor, your call whether to change it or not.
Consistency is important. I replaced all occurrences of "thousands
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping
separator(s)."
Here are the modified changeset:
http://cr.openjdk.java.net/~naoto/8227313/webrev.01/
as well as the modified CSR at the same URL.
Naoto
Best, and have a great Christmas! :-)
Joe
On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:
Hi,
Please review the fix for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8227313
The proposed CSR and changeset are located at:
https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/
The change introduces the new monetary grouping separator in
DecimalFormatSymbols, and it is used if a DecimalFormat instance
designates currency formatting where its pattern includes the
currency symbol (U+00A5). The change makes use of the CLDR data
which provides two distinct grouping separators (one for generic,
the other for currency) in some locales. It also addresses the
similar cases for the decimal separator.
Naoto
Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols
Hi Naoto,
Is there a need for an APINote to note the relationship between the new
get/setMonetaryGroupingSeparator and get/setGroupingSeparator methods.
The new method did state it "May be different from {@code grouping
separator} in some locales", but that may be insufficient. For example,
does setting one method affects the other (seems it should since the
monetaryGroupingSeparator may be initialized by the groupingSeparator as
the impl shows)? If yes, how it's affected?
If no, is there a compatibility concern? In the current impl, the new
get method is used when "isCurrencyFormat" is true while the new set
method doesn't affect the existing 'groupingSeparator'. For existing
applications that have a custom grouping separator set through
setGroupingSeparator, it seems to me the custom separator won't be used
moving onto the new JDK impl.
A minor comment about the definition statement in the following:
Add the following new serializable field
in|java.text.DecimalFormatSymbols|class:
|/** * The grouping separator used when formatting currency values. * *
@serial * @since 15 */ private char monetaryGroupingSeparator;|
and that for the new get method:
Gets the character used for thousands separator for currencies.
While the meanings are clear, they were not as consistent as that
between the existing groupingSeparator and its get method, that is:
Character used for thousands separator.
and then the get method states:
Gets the character used for thousands separator.
But this is minor, your call whether to change it or not.
Best, and have a great Christmas! :-)
Joe
On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:
Hi,
Please review the fix for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8227313
The proposed CSR and changeset are located at:
https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/
The change introduces the new monetary grouping separator in
DecimalFormatSymbols, and it is used if a DecimalFormat instance
designates currency formatting where its pattern includes the currency
symbol (U+00A5). The change makes use of the CLDR data which provides
two distinct grouping separators (one for generic, the other for
currency) in some locales. It also addresses the similar cases for the
decimal separator.
Naoto
Re: [14] RFR(S/T) : 8235866 : bump jtreg requiredVersion to 4.2b16
On 12/12/19 10:46 PM, Igor Ignatyev wrote: @Joe/Lance, were there any reasons why you needed to switch jtreg's requiredVersion back to 4.2b13 in jaxp? are there any reasons which prevent jaxp from switching to jtreg4.2b16 now? I don't recall any specific reason, if any. It would be fine to switch to jtreg4.2b16. Thanks, Joe
Re: RFR 8235238: Parsing a time string ignores any custom TimeZoneNameProvider
+1 -Joe On 12/12/19 2:07 PM, naoto.s...@oracle.com wrote: Sorry, I seem to have posted an old webrev, which included unnecessary retrieval of the generic name (4168-4173 in v.00). Here is the correct webrev: http://cr.openjdk.java.net/~naoto/8235238/webrev.01/ Naoto On 12/12/19 1:37 PM, Roger Riggs wrote: +1 There's quite a bit of work being done to get the eligible stings as part of each parse but it doesn't look easy to re-use it acrosses parses. Roger On 12/12/19 3:42 PM, Joe Wang wrote: On 12/12/19 12:31 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. The original code loops through zoneStrings array, and if the id exists in regionIds, then adds their display names in the tree (4142-4154). This process is not altered with my change. My change made regionIds mutable (line 4127) so that after the loop, it only contains custom ids by calling remove() (line 4144). Thus the new added block will only retrieve names for custom ids. Or am I missing something in your comment? Ok, that's what I was looking for. Thanks for the explanation. The changes look good to me then. Best, Joe Naoto On 12/12/19 11:32 AM, Joe Wang wrote: Hi Naoto, Does the new code block, 4156 - 4174, need a conditional statement, that is when it's for a standard timezon? Before this change, or before a custom TimeZoneNameProvider is attempted, the process didn't need to loop through regionIds to add display names. Thanks, Joe On 12/11/19 1:21 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix for the following issue: https://bugs.openjdk.java.net/browse/JDK-8235238 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8235238/webrev.00/ The fix will retrieve the custom zone names from the time zone name provider, for the custom ZoneRulesProvider implementations. Naoto
Re: RFR 8235238: Parsing a time string ignores any custom TimeZoneNameProvider
On 12/12/19 12:31 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. The original code loops through zoneStrings array, and if the id exists in regionIds, then adds their display names in the tree (4142-4154). This process is not altered with my change. My change made regionIds mutable (line 4127) so that after the loop, it only contains custom ids by calling remove() (line 4144). Thus the new added block will only retrieve names for custom ids. Or am I missing something in your comment? Ok, that's what I was looking for. Thanks for the explanation. The changes look good to me then. Best, Joe Naoto On 12/12/19 11:32 AM, Joe Wang wrote: Hi Naoto, Does the new code block, 4156 - 4174, need a conditional statement, that is when it's for a standard timezon? Before this change, or before a custom TimeZoneNameProvider is attempted, the process didn't need to loop through regionIds to add display names. Thanks, Joe On 12/11/19 1:21 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix for the following issue: https://bugs.openjdk.java.net/browse/JDK-8235238 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8235238/webrev.00/ The fix will retrieve the custom zone names from the time zone name provider, for the custom ZoneRulesProvider implementations. Naoto
Re: RFR 8235238: Parsing a time string ignores any custom TimeZoneNameProvider
Hi Naoto, Does the new code block, 4156 - 4174, need a conditional statement, that is when it's for a standard timezon? Before this change, or before a custom TimeZoneNameProvider is attempted, the process didn't need to loop through regionIds to add display names. Thanks, Joe On 12/11/19 1:21 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix for the following issue: https://bugs.openjdk.java.net/browse/JDK-8235238 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8235238/webrev.00/ The fix will retrieve the custom zone names from the time zone name provider, for the custom ZoneRulesProvider implementations. Naoto
Re: [14] RFR: 8222756: Plural support in CompactNumberFormat
+1, looks good! Best regards, Joe On 12/5/19 12:13 PM, Roger Riggs wrote: Hi Naoto, Thanks for the updates. Looks fine. Roger On 12/5/19 1:49 PM, naoto.s...@oracle.com wrote: Thanks, Joe and Roger, for the reviews. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8222756/webrev.02/ These are the changes since v.01: CompactNumberFormat.java: - Reflected the CSR changes (thank you, JoeD, for the quick turnaround!), and misc typos in the spec. - Added length limitation to the "pluralRules" argument in the new constructor. Throws IllegalArgumentException if the input is too long (>2,048 chars). - Added validation to plural rules, and throws IllegalArgumentException if the given rules' syntax is incorrect. - Consolidated the same pattern to get the affixes into a utility method (getAffix()) - Directly find NaN and Infinity in the number string, parse the number with regex otherwise. TestEquality.java: Tidied the test array up. CLDRConverter.java: Added String::trim so that the generated PluralRules.java will not have extra spaces. TestPlurals.java: Added test cases for invalid cases, i.e., invalid syntax and length limit exceeding rules in the constructor. Not addressed: - ResourceBundleGenerator.java exists in the webrev as it does have a modification (only seen in "patch" link, as the mod is only the indentation) - Using "==" for double values: As Roger mentioned, it is in fact comparing integers (the only reason to use double is to deal with NaN and Infinity) - Possible performance improvement: Could be addressed later if it is regarded as an issue. But since the effective plural rules are simple/short enough, I would not expect it as a problem. Naoto On 11/26/19 1:35 PM, naoto.s...@oracle.com wrote: I modified CompactNumberFormat.java to simplify the syntax parsing: https://cr.openjdk.java.net/~naoto/8222756/webrev.01/ Please review this webrev instead. Naoto On 11/25/19 1:16 PM, naoto.s...@oracle.com wrote: Hello, CompactNumberFormat has been added since JDK 12 to support compact number formatting, such as 10_000 being formatted as "10K." [1] It works fine for English, however, not for other languages that take plural forms in formatted number prefixes/suffixes. In order to fix this, I filed the following CSR to extend the current CompactNumberFormat spec: https://bugs.openjdk.java.net/browse/JDK-8232633 It basically accommodates the plural prefix/suffix forms into the existing compact patterns array, so that the existing compact number format works compatibly. The plural rules are solely based on the CLDR's plural language rules [2] Here is the implementation of the CSR: https://cr.openjdk.java.net/~naoto/8222756/webrev.00/ Please review the CSR as well as its implementation. Naoto [1] https://bugs.openjdk.java.net/browse/JDK-8177552 [2] https://unicode.org/reports/tr35/tr35-numbers.html#Language_Plural_Rules
Re: [14] RFR: 8222756: Plural support in CompactNumberFormat
Hi Naoto, Looks good. I understand you'll update the webrev (with the added statement to readObject) once the CSR is approved. ResourceBundleGenerator.java might have been accidentally touched as there's no change there. I wonder if you need to guard the pluralRules input since you're building a Map with a split. While it normally won't be a problem if the factory methods are used, there's still a chance CompactNumberFormat is constructed directly (e.g. with a custom format). Best, Joe On 11/26/19 1:35 PM, naoto.s...@oracle.com wrote: I modified CompactNumberFormat.java to simplify the syntax parsing: https://cr.openjdk.java.net/~naoto/8222756/webrev.01/ Please review this webrev instead. Naoto On 11/25/19 1:16 PM, naoto.s...@oracle.com wrote: Hello, CompactNumberFormat has been added since JDK 12 to support compact number formatting, such as 10_000 being formatted as "10K." [1] It works fine for English, however, not for other languages that take plural forms in formatted number prefixes/suffixes. In order to fix this, I filed the following CSR to extend the current CompactNumberFormat spec: https://bugs.openjdk.java.net/browse/JDK-8232633 It basically accommodates the plural prefix/suffix forms into the existing compact patterns array, so that the existing compact number format works compatibly. The plural rules are solely based on the CLDR's plural language rules [2] Here is the implementation of the CSR: https://cr.openjdk.java.net/~naoto/8222756/webrev.00/ Please review the CSR as well as its implementation. Naoto [1] https://bugs.openjdk.java.net/browse/JDK-8177552 [2] https://unicode.org/reports/tr35/tr35-numbers.html#Language_Plural_Rules
Re: RFR [14/java.xml] 8233548: Update CUP to v0.11b
Thanks Lance! -Joe On 11/21/19 3:59 PM, Lance Andersen wrote: Hi Joe, Looks OK overall. Best Lance On Nov 20, 2019, at 4:48 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Hi, Please review an update to Java CUP. This is an effort to move to the latest version, v0.11b. JCUP is used by Xalan to generate XPathParser. Included in the JDK are runtime classes and XPathParser. In CUP 0.11b, the main additions to the runtime were SymbolFactory and ComplexSymbol that were used to report a bit more details (locations) about the parsing process. They were not used by Xalan nor the JDK since the benefit of these additions were in error reporting, but Xalan and JDK used their own reporting mechanism. I could have removed the JCUP error report process altogether, but decided to keep it in case it might be used for debugging. The main change for this update therefore is to the XPathParser itself. Since JCUP has been stable, and no major Xalan update in sight, I took the liberty to re-format the whole class, plus some cleanup (mainly boxing/unboxing), that constitute the changes before the inner class parser_actions. Within parser_actions, the main change is that the 0.11b fixed the order of the switch block. Other than that, I removed many unused variables, and also formatted the long lines. There were no other material changes. So there's no new test, all existing tests (including JCK) passed. JBS: https://bugs.openjdk.java.net/browse/JDK-8233548 webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8233548/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8233548: Update CUP to v0.11b
Hi, Please review an update to Java CUP. This is an effort to move to the latest version, v0.11b. JCUP is used by Xalan to generate XPathParser. Included in the JDK are runtime classes and XPathParser. In CUP 0.11b, the main additions to the runtime were SymbolFactory and ComplexSymbol that were used to report a bit more details (locations) about the parsing process. They were not used by Xalan nor the JDK since the benefit of these additions were in error reporting, but Xalan and JDK used their own reporting mechanism. I could have removed the JCUP error report process altogether, but decided to keep it in case it might be used for debugging. The main change for this update therefore is to the XPathParser itself. Since JCUP has been stable, and no major Xalan update in sight, I took the liberty to re-format the whole class, plus some cleanup (mainly boxing/unboxing), that constitute the changes before the inner class parser_actions. Within parser_actions, the main change is that the 0.11b fixed the order of the switch block. Other than that, I removed many unused variables, and also formatted the long lines. There were no other material changes. So there's no new test, all existing tests (including JCK) passed. JBS: https://bugs.openjdk.java.net/browse/JDK-8233548 webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8233548/webrev/ Thanks, Joe
Re: RFR [14/java.xml] 8233686: XML transformer uses excessive amount of memory
Thanks for looking at the changeset. A small change to eliminate unnecessary object allocations, that's the whole purpose, and what makes it worth the effort. As for the exact numbers (of how much it might save), it depends on users/customers' environment. In terms of keeping in sync, we do that for formal releases. Best, Joe On 11/8/19 4:33 AM, Bernd Eckenfels wrote: This does save object allocations and churn, not memory footprint I guess. The namespace mapping contains multiple stacks (with object arrays) and a hashtable and initialized records, so it seems to allocate a few kb on every node visited. (But 100MB allocation does sound like a very constructed case) BTW the thing I wondered, is there a process to keep xerces in sync? Gruss Bernd -- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von Vyom Tiwari Gesendet: Freitag, November 8, 2019 11:14 AM An: Joe Wang Cc: core-libs-dev Betreff: Re: RFR [14/java.xml] 8233686: XML transformer uses excessive amount of memory Hi Joe, Fix looks OK to me , but i am not able to understand how come "NamespaceMappings" instance can increase memory uses from (20MB to 140MB ). Current scope of "ns" is "case Node.ELEMENT_NODE:" block and "NamespaceMapping" seems to be very lightweight class. Thanks, Vyom On Fri, Nov 8, 2019 at 12:33 AM Joe Wang wrote: Please review a quick fix that reduces unnecessary object allocations. JBS: https://bugs.openjdk.java.net/browse/JDK-8233686 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8233686/webrev/ Thanks, Joe -- Thanks, Vyom
RFR [14/java.xml] 8233686: XML transformer uses excessive amount of memory
Please review a quick fix that reduces unnecessary object allocations. JBS: https://bugs.openjdk.java.net/browse/JDK-8233686 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8233686/webrev/ Thanks, Joe
Re: [14] RFR: 8233579: DateFormatSymbols.getShortMonths() return wrong string on es_CL, es_CO locales
That sounds good. JIRA makes sense. Best, Joe On 11/6/19 6:34 PM, naoto.s...@oracle.com wrote: Hi Joe, Thanks for the review! I tried to draft some comments, but thinking of it, it could be confusing to leave a comment for a removed (wrong) code. So instead of leaving code comments, I modified the JIRA issue mentioning it, so that maintainers later could find out the rationale. Naoto On 11/6/19 6:21 PM, Joe Wang wrote: Looks good to me. It might be good to leave a note to the method about the change (e.g. why it no longer substitutes). -Joe On 11/6/19 1:45 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8233579 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8233579/webrev.00/ The problem was in the CLDR converter where it substitutes the context dependent style names with stand alone style names, if the former set of names are missing in a locale bundle. Corrected to not substitute, thus inherit from the parent bundles. Naoto
Re: [14] RFR: 8233579: DateFormatSymbols.getShortMonths() return wrong string on es_CL, es_CO locales
Looks good to me. It might be good to leave a note to the method about the change (e.g. why it no longer substitutes). -Joe On 11/6/19 1:45 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8233579 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8233579/webrev.00/ The problem was in the CLDR converter where it substitutes the context dependent style names with stand alone style names, if the former set of names are missing in a locale bundle. Corrected to not substitute, thus inherit from the parent bundles. Naoto
Re: RFR(XS): 8232713: Update BCEL version to 6.3.1 in license file
+1. Thanks Aleksei! -Joe On 10/21/19 6:09 AM, Aleks Efimov wrote: Hi, The BCEL version has been updated to 6.3.1 by JDK-8224157 [1]. The BCEL license file needs to be updated to include the correct version. Webrev: http://cr.openjdk.java.net/~aefimov/8232713/00 JBS: https://bugs.openjdk.java.net/browse/JDK-8232713 With Best Regards, Aleksei [1] https://bugs.openjdk.java.net/browse/JDK-8224157
Re: RFR [14/java.xml] 8016914: CoreDocumentImpl.setXmlVersion NPE
Thanks Lance! On 9/27/19 10:47 AM, Lance Andersen wrote: Hi Joe, The change looks fine as does the test. Best Lance On Sep 27, 2019, at 12:29 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a fix to the NPE issue. This is part of the work on the XML declaration related issues. JBS: https://bugs.openjdk.java.net/browse/JDK-8016914 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8016914/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8016914: CoreDocumentImpl.setXmlVersion NPE
Please review a fix to the NPE issue. This is part of the work on the XML declaration related issues. JBS: https://bugs.openjdk.java.net/browse/JDK-8016914 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8016914/webrev/ Thanks, Joe
Re: RFR [14/java.xml] 8231083: Clarify SAX documentation
Thanks Alan! Joe On 9/20/19 10:03 AM, Alan Bateman wrote: On 20/09/2019 17:46, Joe Wang wrote: Thanks Alan! Here's the updated webrev after changing the apiNote as you suggested: http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev02/index.html Looks good.
Re: RFR [14/java.xml] 8231083: Clarify SAX documentation
Thanks Alan! Here's the updated webrev after changing the apiNote as you suggested: http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev02/index.html -Joe On 9/20/19 9:07 AM, Alan Bateman wrote: On 20/09/2019 06:15, Joe Wang wrote: Thanks Lance! Yes, saw them typos :-) Also removed the extra space in apiNote. Updated webrev below, with removing the text in the javadoc instead of moving to the header. http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev/index.html Just a minor comment on the @apiNote where it reads "was introduced in Java SE 1.4 and continues to be maintained as part of Java SE". It might be simpler to just say that "has been defined by Java SE since 1.4" and leave out any mention of being maintained. -Alan.
Re: RFR [14/java.xml] 8231083: Clarify SAX documentation
Thanks Lance! It's a cleanup on top of cleanup :-) On 9/20/19 4:13 AM, Lance Andersen wrote: Round 2 even looks cleaner :-) On Sep 20, 2019, at 1:15 AM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Thanks Lance! Yes, saw them typos :-) Also removed the extra space in apiNote. Updated webrev below, with removing the text in the javadoc instead of moving to the header. http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev/index.html -Joe On 9/19/19 5:18 PM, Lance Andersen wrote: Hi Joe, Overall this looks good and also cleans up a couple of typos :-) One nit in both package-info @apiNote, you will notice an extra space before the was which could be removed before you push Best Lance On Sep 19, 2019, at 8:00 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a follow-up doc clarification patch after 8230814 [1]. In this patch, the statement with a reference to the SAX project is moved to an apiNote in package/sub-package description to reflect the fact that it is a historical note in nature. The license related text that appears in the class description of every SAX class is removed and consolidated with the existing text in the license header. JBS: https://bugs.openjdk.java.net/browse/JDK-8231083 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev/index.html [1] https://bugs.openjdk.java.net/browse/JDK-8230814 Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: RFR [14/java.xml] 8231083: Clarify SAX documentation
Thanks Lance! Yes, saw them typos :-) Also removed the extra space in apiNote. Updated webrev below, with removing the text in the javadoc instead of moving to the header. http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev/index.html -Joe On 9/19/19 5:18 PM, Lance Andersen wrote: Hi Joe, Overall this looks good and also cleans up a couple of typos :-) One nit in both package-info @apiNote, you will notice an extra space before the was which could be removed before you push Best Lance On Sep 19, 2019, at 8:00 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a follow-up doc clarification patch after 8230814 [1]. In this patch, the statement with a reference to the SAX project is moved to an apiNote in package/sub-package description to reflect the fact that it is a historical note in nature. The license related text that appears in the class description of every SAX class is removed and consolidated with the existing text in the license header. JBS: https://bugs.openjdk.java.net/browse/JDK-8231083 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev/index.html [1] https://bugs.openjdk.java.net/browse/JDK-8230814 Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8231083: Clarify SAX documentation
Please review a follow-up doc clarification patch after 8230814 [1]. In this patch, the statement with a reference to the SAX project is moved to an apiNote in package/sub-package description to reflect the fact that it is a historical note in nature. The license related text that appears in the class description of every SAX class is removed and consolidated with the existing text in the license header. JBS: https://bugs.openjdk.java.net/browse/JDK-8231083 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8231083/webrev/index.html [1] https://bugs.openjdk.java.net/browse/JDK-8230814 Thanks, Joe
Re: RFR [14/java.xml] 8230814: Enable SAX ContentHandler to handle XML Declaration
On 9/17/19 3:00 AM, Alan Bateman wrote: On 16/09/2019 19:25, Joe Wang wrote: : I deliberately left the javadoc in the SAX package as they are. But I agree it may be worth it to address this aspect of the document. I suggest we do so with a separate doc-only request[1] to clarify the relationship with the SAX project, likely adding a short note in the package description and removing all other references. If you are okay, I'll keep this changeset the way it is, limiting it to the new method. A separate issue is fine but I think important to do it in the same release where the APIs diverge to avoid confusing the reader (by redirecting them to stale API docs). Agree, will do it next while we all are here on the issue. -Joe -Alan
Re: RFR [14/java.xml] 8230814: Enable SAX ContentHandler to handle XML Declaration
On 9/14/19 1:08 AM, Alan Bateman wrote: On 13/09/2019 21:50, Joe Wang wrote: : It can be said that the SAX project, in terms of the API work, was dead a long time ago. There hasn't been any updates/changes since SAX 2.0.2 released in 2004[1]. SAX is in public domain [2]. Sun/Oracle incorporated SAX2 in Java SE with a GPL license. I assume we're free to make changes. Please let me know if you think otherwise. I'm not objecting to notifying the content handler of declarations. I'm also not discussing licenses. I'm mostly concerned that ContentHandler and all the other classes in this API point the reader to the SAX project as the place to go for documentation and more information. Has there been any effort to find a contact for the SAX project on soucreforge and get them to put an EOL notice on the main page? Alternatively, if the SAX API in Java SE is getting a second wind then maybe the links to the SAX project could be reduced to a historical note. I deliberately left the javadoc in the SAX package as they are. But I agree it may be worth it to address this aspect of the document. I suggest we do so with a separate doc-only request[1] to clarify the relationship with the SAX project, likely adding a short note in the package description and removing all other references. If you are okay, I'll keep this changeset the way it is, limiting it to the new method. [1] https://bugs.openjdk.java.net/browse/JDK-8231083 -Joe -Alan
Re: RFR [14/java.xml] 8230814: Enable SAX ContentHandler to handle XML Declaration
On 9/13/19 12:20 PM, Alan Bateman wrote: On 13/09/2019 19:53, Joe Wang wrote: Please review a new method addition to SAX ContentHandler that allows a SAX parser to send back information about the declaration of the input document. JBS: https://bugs.openjdk.java.net/browse/JDK-8230814 CSR: https://bugs.openjdk.java.net/browse/JDK-8230824 This adds a method to an API that was originally developed by the SAX project. Can you summarize the relationship to that project? Is that project dead and the API subsumed by Java SE? Just asking as I don't recall API additions being done here (I remember the work in JDK-8152912 to deprecate XMLReaderFactory but I don't recall any additional or removals). It can be said that the SAX project, in terms of the API work, was dead a long time ago. There hasn't been any updates/changes since SAX 2.0.2 released in 2004[1]. SAX is in public domain [2]. Sun/Oracle incorporated SAX2 in Java SE with a GPL license. I assume we're free to make changes. Please let me know if you think otherwise. [1] http://sax.sourceforge.net/ [2] http://sax.sourceforge.net/copying.html Thanks Joe -Alan.
RFR [14/java.xml] 8230814: Enable SAX ContentHandler to handle XML Declaration
Please review a new method addition to SAX ContentHandler that allows a SAX parser to send back information about the declaration of the input document. JBS: https://bugs.openjdk.java.net/browse/JDK-8230814 CSR: https://bugs.openjdk.java.net/browse/JDK-8230824 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8230814/webrev/ Thanks, Joe
Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth
+1, looks good to me. Best regards, Joe On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8230136 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8230136/webrev.00/ The fix is to correct the condition of the invalid case, as suggested in the bug report. Naoto
Re: RFR [14/java.xml] 8228854: Default ErrorListener reports warnings and errors to the console
Thanks Lance! I did a re-run of all tests and they all passed. -Joe On 9/4/19 12:12 PM, Lance Andersen wrote: Hi Joe +1 On Sep 4, 2019, at 1:30 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a patch that removes some warnings and errors printed out to the console. JBS: https://bugs.openjdk.java.net/browse/JDK-8228854 CSR: https://bugs.openjdk.java.net/browse/JDK-8228973 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8228854/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8228854: Default ErrorListener reports warnings and errors to the console
Please review a patch that removes some warnings and errors printed out to the console. JBS: https://bugs.openjdk.java.net/browse/JDK-8228854 CSR: https://bugs.openjdk.java.net/browse/JDK-8228973 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8228854/webrev/ Thanks, Joe
Re: RFR [14/java.xml] 8230094: CCE in createXMLEventWriter(Result) over an arbitrary XMLStreamWriter
On 8/28/19 8:12 AM, Lance Andersen wrote: Hi Joe, The change and test look OK Thanks Lance! On Aug 27, 2019, at 1:20 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a patch for an issue where a custom impl of XMLStreamWriter caused a CCE. The problem was that our implementation was changed to an internally extended interface. We could consider making it external, but for now, we'll do with a check. Note that there's no material change in XMLOutputFactoryImpl.java except a minor one at line 160. took a moment to spot the change here :-) It's hidden in there ;-) Best, Joe Best Lance JBS: https://bugs.openjdk.java.net/browse/JDK-8230094 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8230094/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8230094: CCE in createXMLEventWriter(Result) over an arbitrary XMLStreamWriter
Please review a patch for an issue where a custom impl of XMLStreamWriter caused a CCE. The problem was that our implementation was changed to an internally extended interface. We could consider making it external, but for now, we'll do with a check. Note that there's no material change in XMLOutputFactoryImpl.java except a minor one at line 160. JBS: https://bugs.openjdk.java.net/browse/JDK-8230094 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8230094/webrev/ Thanks, Joe
Re: RFR (14) [testbug]: runs zero test, 8230002 and 8230010
Hi Frank, Thanks for fixing this. How did you find this out? Are you running some kind of tools? I hope that's the case since then we could be sure there were no other cases in the test suite. The patch looks good. It's fortunate we haven't broken anything ;-) The test passed just fine. Best, Joe On 8/27/19 2:06 AM, Frank Yuan wrote: Hi all We found 2 jaxp tests, which didn't run indeed. Bugs: https://bugs.openjdk.java.net/browse/JDK-8230002 Annotation @Test was missed in TestNG test. https://bugs.openjdk.java.net/browse/JDK-8230010 The old test was left during it was converted to TestNG test. Webrev: http://cr.openjdk.java.net/~fyuan/8230002_8230010/webrev.00/ Thanks Frank
Re: RFR [14/java.xml] 8229388: ErrorHandler and ContentHandler contain ambiguous/unfinished specification
Thanks Lance! On 8/22/19 9:56 AM, Lance Andersen wrote: Hi Joe, This looks good overall. Best Lance On Aug 21, 2019, at 9:31 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a specification claraficaiton/doc-only change to ErrorHandler and ContentHandler. JBS: https://bugs.openjdk.java.net/browse/JDK-8229388 CSR: https://bugs.openjdk.java.net/browse/JDK-8229738 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8229388/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8229388: ErrorHandler and ContentHandler contain ambiguous/unfinished specification
Please review a specification claraficaiton/doc-only change to ErrorHandler and ContentHandler. JBS: https://bugs.openjdk.java.net/browse/JDK-8229388 CSR: https://bugs.openjdk.java.net/browse/JDK-8229738 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8229388/webrev/ Thanks, Joe
Re: RFR: 8229485: Add decrementExact(), incrementExact(), and negateExact() to java.lang.StrictMath
Hi Daniel, That makes sense. So it meant to say "converts the long to int only if it's within the range of an integer" (to be "exact"). It may then add "throws an exception if not". But the original is fine, just my 2 cents. Best, Joe On 8/14/19 12:21 PM, Daniel Fuchs wrote: Hi Joe, On 14/08/2019 20:15, Joe Wang wrote: The 2nd part is not necessary as that's what the "@throws" tag is for. Not withstanding the fact that this is a copy of the API doc from java.lang.Math, I'd argue that the second part is quite important. It's why the method is called exact, and is probably the only reason why you would call that method in the first place. So I think the 2nd part must be in the synopsis too. best regards, -- daniel On 8/14/19 9:01 AM, Julia Boes wrote: > > Hi, > > This fix adds decrementExact(), incrementExact(), and negateExact() to java.lang.StrictMath. The methods were added to java.lang.Math previously [1] and should have been added to java.lang.StrictMath for consistency. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8229485 > > Webrev: http://cr.openjdk.java.net/~dfuchs/jboes/8229485/webrev.01/ > > CSR: https://bugs.openjdk.java.net/browse/JDK-8229702 > > Thanks, > > Julia > > [1] https://bugs.openjdk.java.net/browse/JDK-8022109
Re: RFR: 8229485: Add decrementExact(), incrementExact(), and negateExact() to java.lang.StrictMath
while you're here, could you please replace semicolon with comma:
916 * Returns the value of the {@code long} argument;
917 * throwing an exception if the value overflows an {@code int}.
It seems to me the first sentence shall be the synopsis. The 2nd part is
not necessary as that's what the "@throws" tag is for. If it has to be
there, should it be "throws" instead of "throwing" (I'm not a native
speaker :-) )
Cheers,
Joe
With kind regards,
Ivan
On 8/14/19 9:01 AM, Julia Boes wrote:
Hi,
This fix adds decrementExact(), incrementExact(), and negateExact()
to java.lang.StrictMath. The methods were added to java.lang.Math
previously [1] and should have been added to java.lang.StrictMath for
consistency.
Bug: https://bugs.openjdk.java.net/browse/JDK-8229485
Webrev: http://cr.openjdk.java.net/~dfuchs/jboes/8229485/webrev.01/
CSR: https://bugs.openjdk.java.net/browse/JDK-8229702
Thanks,
Julia
[1] https://bugs.openjdk.java.net/browse/JDK-8022109
Re: RFR [14/java.xml] 8068376: Validator fails valid XML files due to String == in XSD validator code
Thanks Lance! -Joe On 7/25/19 4:04 PM, Lance Andersen wrote: Hi Joe The change looks reasonable :-) On Jul 25, 2019, at 6:28 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a quite fix in a validation source where a String comparison was made as references. JBS: https://bugs.openjdk.java.net/browse/JDK-8068376 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8068376/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8068376: Validator fails valid XML files due to String == in XSD validator code
Please review a quite fix in a validation source where a String comparison was made as references. JBS: https://bugs.openjdk.java.net/browse/JDK-8068376 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8068376/webrev/ Thanks, Joe
Re: [14] RFR: 8212970: TZ database in "vanguard" format support
Looks all good Naoto :-) -Joe On 7/25/19 2:35 PM, naoto.s...@oracle.com wrote: Hi Joe, Yes, I only removed not in-use files, i.e., 2 & 4. I sent the previous email just after confirming that all tests (open/closed) passed on a platform :-) Naoto On 7/25/19 2:24 PM, Joe Wang wrote: Hi Naoto, The legacy trap :-) Relevant files: 1. make/data/tzdata/jdk11_backward 2. test/jdk/sun/util/calendar/zi/tzdata/jdk11_backward 3. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_backward 4. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_full_backward I see you reverted changes to (1) plus removing (2) and (4). (3) and (4) differs slightly, but contains the changes previously made in (1). Probably not something to worry about since only (3) is referenced, and (4) not. Just wonder whether you've checked on this one. I assume your build and test passed without any issues. Best, Joe On 7/25/19 1:04 PM, naoto.s...@oracle.com wrote: Hi Roger, On 7/25/19 7:47 AM, Roger Riggs wrote: Hi Naoto, TestZoneInfo310.java: With the composition of the tzdir path up and over to the make directory for the tzdir it might be useful to do an explicit check that the directory exists. That way if the directory structure on the build side changes, there will be a test failure makine it obvious that the dependency has changed. If the input tz data files, either in "test" tree or "make" tree, cannot be located, the test will fail which effectively reports if there is a repo structure change. So I believe it is ok as it is. Aside from it, the latest changes to eliminate the duplicates caused that regression test fail. The reason was that there were actually two "jdk11_backward" data files each in "tzdata" and "tzdata_jdk" test directories, and the contents differ! I am not sure the reason why there are two files this way (seems to be so for years), so I reverted that exact file as before. Here is the webrev reflected that: http://cr.openjdk.java.net/~naoto/8212970/webrev.12/ Naoto Looks fine. Thanks, Roger On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. On 7/24/19 2:57 PM, Joe Wang wrote: Hi Naoto, The method findNegativeSavings method in TzdbZoneRulesProvider.java states that it "Find the minimum negative savings". While the result is correct since the rules all have the same value for SAVE, I wonder if that's ideal conceptually. Given a start LDT, shouldn't it be looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 1989 vs 1981 - max)?. I believe it is working as such. The end year is retrieved within the method (line 879) and only the minimum negative saving values within the window is filtered. NegativeDSTTest verifies the tzdata, that is the adjusted data after import, is that correct? I wonder a comment and a bit of details in the test summary would be helpful since there is no negative data in the test itself. Good point. It is confusing. I supplied summary text in the test (also the similar line in TestZoneRules.java) Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8212970/webrev.11/ Naoto Best, Joe On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following enhancement: https://bugs.openjdk.java.net/browse/JDK-8212970 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8212970/webrev.09/ This change aims to support the "vanguard" IANA time zone data format, which uses the negative savings and transition time beyond a day period. The change basically translates those negative savings and transition times, such as 25:00, into the ones that the current JDK recognizes, then produces the data file "tzdb.dat" at the build time. At the run time, the data file is read and interpreted as before. This way the produced tzdb.dat is compatible with the prior JDK releases so that the TZ Updater can also distribute it as a time zone update. I have also refactored redundant copy of ZoneRules file in the build directory, by dynamically importing the file under src. Thus some build related files are modified. I am hoping folks on the build-dev can review those changes. Naoto
Re: [14] RFR: 8212970: TZ database in "vanguard" format support
Hi Naoto, The legacy trap :-) Relevant files: 1. make/data/tzdata/jdk11_backward 2. test/jdk/sun/util/calendar/zi/tzdata/jdk11_backward 3. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_backward 4. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_full_backward I see you reverted changes to (1) plus removing (2) and (4). (3) and (4) differs slightly, but contains the changes previously made in (1). Probably not something to worry about since only (3) is referenced, and (4) not. Just wonder whether you've checked on this one. I assume your build and test passed without any issues. Best, Joe On 7/25/19 1:04 PM, naoto.s...@oracle.com wrote: Hi Roger, On 7/25/19 7:47 AM, Roger Riggs wrote: Hi Naoto, TestZoneInfo310.java: With the composition of the tzdir path up and over to the make directory for the tzdir it might be useful to do an explicit check that the directory exists. That way if the directory structure on the build side changes, there will be a test failure makine it obvious that the dependency has changed. If the input tz data files, either in "test" tree or "make" tree, cannot be located, the test will fail which effectively reports if there is a repo structure change. So I believe it is ok as it is. Aside from it, the latest changes to eliminate the duplicates caused that regression test fail. The reason was that there were actually two "jdk11_backward" data files each in "tzdata" and "tzdata_jdk" test directories, and the contents differ! I am not sure the reason why there are two files this way (seems to be so for years), so I reverted that exact file as before. Here is the webrev reflected that: http://cr.openjdk.java.net/~naoto/8212970/webrev.12/ Naoto Looks fine. Thanks, Roger On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. On 7/24/19 2:57 PM, Joe Wang wrote: Hi Naoto, The method findNegativeSavings method in TzdbZoneRulesProvider.java states that it "Find the minimum negative savings". While the result is correct since the rules all have the same value for SAVE, I wonder if that's ideal conceptually. Given a start LDT, shouldn't it be looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 1989 vs 1981 - max)?. I believe it is working as such. The end year is retrieved within the method (line 879) and only the minimum negative saving values within the window is filtered. NegativeDSTTest verifies the tzdata, that is the adjusted data after import, is that correct? I wonder a comment and a bit of details in the test summary would be helpful since there is no negative data in the test itself. Good point. It is confusing. I supplied summary text in the test (also the similar line in TestZoneRules.java) Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8212970/webrev.11/ Naoto Best, Joe On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following enhancement: https://bugs.openjdk.java.net/browse/JDK-8212970 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8212970/webrev.09/ This change aims to support the "vanguard" IANA time zone data format, which uses the negative savings and transition time beyond a day period. The change basically translates those negative savings and transition times, such as 25:00, into the ones that the current JDK recognizes, then produces the data file "tzdb.dat" at the build time. At the run time, the data file is read and interpreted as before. This way the produced tzdb.dat is compatible with the prior JDK releases so that the TZ Updater can also distribute it as a time zone update. I have also refactored redundant copy of ZoneRules file in the build directory, by dynamically importing the file under src. Thus some build related files are modified. I am hoping folks on the build-dev can review those changes. Naoto
Re: [14] RFR: 8212970: TZ database in "vanguard" format support
Thanks Naoto. Looks good. -Joe On 7/24/19 3:24 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. On 7/24/19 2:57 PM, Joe Wang wrote: Hi Naoto, The method findNegativeSavings method in TzdbZoneRulesProvider.java states that it "Find the minimum negative savings". While the result is correct since the rules all have the same value for SAVE, I wonder if that's ideal conceptually. Given a start LDT, shouldn't it be looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 1989 vs 1981 - max)?. I believe it is working as such. The end year is retrieved within the method (line 879) and only the minimum negative saving values within the window is filtered. NegativeDSTTest verifies the tzdata, that is the adjusted data after import, is that correct? I wonder a comment and a bit of details in the test summary would be helpful since there is no negative data in the test itself. Good point. It is confusing. I supplied summary text in the test (also the similar line in TestZoneRules.java) Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8212970/webrev.11/ Naoto Best, Joe On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following enhancement: https://bugs.openjdk.java.net/browse/JDK-8212970 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8212970/webrev.09/ This change aims to support the "vanguard" IANA time zone data format, which uses the negative savings and transition time beyond a day period. The change basically translates those negative savings and transition times, such as 25:00, into the ones that the current JDK recognizes, then produces the data file "tzdb.dat" at the build time. At the run time, the data file is read and interpreted as before. This way the produced tzdb.dat is compatible with the prior JDK releases so that the TZ Updater can also distribute it as a time zone update. I have also refactored redundant copy of ZoneRules file in the build directory, by dynamically importing the file under src. Thus some build related files are modified. I am hoping folks on the build-dev can review those changes. Naoto
Re: [14] RFR: 8212970: TZ database in "vanguard" format support
Hi Naoto, The method findNegativeSavings method in TzdbZoneRulesProvider.java states that it "Find the minimum negative savings". While the result is correct since the rules all have the same value for SAVE, I wonder if that's ideal conceptually. Given a start LDT, shouldn't it be looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 1989 vs 1981 - max)?. NegativeDSTTest verifies the tzdata, that is the adjusted data after import, is that correct? I wonder a comment and a bit of details in the test summary would be helpful since there is no negative data in the test itself. Best, Joe On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following enhancement: https://bugs.openjdk.java.net/browse/JDK-8212970 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8212970/webrev.09/ This change aims to support the "vanguard" IANA time zone data format, which uses the negative savings and transition time beyond a day period. The change basically translates those negative savings and transition times, such as 25:00, into the ones that the current JDK recognizes, then produces the data file "tzdb.dat" at the build time. At the run time, the data file is read and interpreted as before. This way the produced tzdb.dat is compatible with the prior JDK releases so that the TZ Updater can also distribute it as a time zone update. I have also refactored redundant copy of ZoneRules file in the build directory, by dynamically importing the file under src. Thus some build related files are modified. I am hoping folks on the build-dev can review those changes. Naoto
Re: RFR [14/java.xml] 8157830: Errors in XSLT stylesheet are not dispatched correctly to ErrorListener
Thanks Lance! On 7/18/19 3:06 PM, Lance Andersen wrote: +1 On Jul 18, 2019, at 1:40 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a patch to eliminate the [Fatal Error] message printed out to the console. Whether the parser should have written the error out in the first place is up to debate, not covered in this patch, but when the request is for the Transformer to handle errors through a registered ErrorListener, all errors should have been passed onto the ErrorListener. The fix is to set a proxy handler to the parser that simply hands the errors over to the Transformer's ErrorListener. JBS: https://bugs.openjdk.java.net/browse/JDK-8157830 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8157830/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8157830: Errors in XSLT stylesheet are not dispatched correctly to ErrorListener
Please review a patch to eliminate the [Fatal Error] message printed out to the console. Whether the parser should have written the error out in the first place is up to debate, not covered in this patch, but when the request is for the Transformer to handle errors through a registered ErrorListener, all errors should have been passed onto the ErrorListener. The fix is to set a proxy handler to the parser that simply hands the errors over to the Transformer's ErrorListener. JBS: https://bugs.openjdk.java.net/browse/JDK-8157830 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8157830/webrev/ Thanks, Joe
Re: RFR [14/java.xml] 8176447: javax.xml.validation.Validator validates incorrectly on uniqueness constraint
Thanks Lance! On 7/16/19 12:20 PM, Lance Andersen wrote: Looks OK Joe On Jul 16, 2019, at 12:34 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a fix for the uniqueness constraint. The fix is at line 403 where the variable for tracking matches is reset. Without the reset, subsequent matches would have been skipped, thus not catching duplicate values. As the report's workaround indicated, this issue was fixed in Apache Xerces. This patch now brings the class up-to-date with the upstream source. All tests passed. JBS: https://bugs.openjdk.java.net/browse/JDK-8176447 webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8176447/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8176447: javax.xml.validation.Validator validates incorrectly on uniqueness constraint
Please review a fix for the uniqueness constraint. The fix is at line 403 where the variable for tracking matches is reset. Without the reset, subsequent matches would have been skipped, thus not catching duplicate values. As the report's workaround indicated, this issue was fixed in Apache Xerces. This patch now brings the class up-to-date with the upstream source. All tests passed. JBS: https://bugs.openjdk.java.net/browse/JDK-8176447 webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8176447/webrev/ Thanks, Joe
Re: RFR [14]: 8227438: [TESTLIB] Determine if file exists by Files.exists in function FileUtils.deleteFileIfExistsWithRetry
+1
-Joe
On 7/11/19 11:32 PM, Frank Yuan wrote:
Hi
Would you like to review the following patch for
Bug: https://bugs.openjdk.java.net/browse/JDK-8227438
--- a/test/lib/jdk/test/lib/util/FileUtils.java Thu Jul 11 15:58:54 2019
+
+++ b/test/lib/jdk/test/lib/util/FileUtils.java Fri Jul 12 13:33:30 2019 +0800
@@ -96,7 +96,7 @@
*/
public static void deleteFileIfExistsWithRetry(Path path) throws
IOException {
try {
-if (Files.exists(path)) {
+if (!Files.notExists(path)) {
deleteFileWithRetry0(path);
}
} catch (InterruptedException x) {
Files.exists returns false if the existence cannot be determined, so replace it with
"!Files.notExists" like the change in
JDK-8184961.
Thanks
Frank
Re: RFR [14/java.xml] 8178843: A bug in an inner loop in MethodGenerator's getLocals method
Thanks Lance! -Joe On 7/10/19 6:38 PM, Lance Andersen wrote: +1 On Jul 10, 2019, at 7:57 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a cleanup that removes unused code. This code has two errors, one as indicated in the bug report, another attempting to loop through allVarsEverDeclared that was just created (meaning size=0). It just ensures again that the code was never used. JBS: https://bugs.openjdk.java.net/browse/JDK-8178843 webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8178843/webrev/ All tests passed. Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 8178843: A bug in an inner loop in MethodGenerator's getLocals method
Please review a cleanup that removes unused code. This code has two errors, one as indicated in the bug report, another attempting to loop through allVarsEverDeclared that was just created (meaning size=0). It just ensures again that the code was never used. JBS: https://bugs.openjdk.java.net/browse/JDK-8178843 webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8178843/webrev/ All tests passed. Thanks, Joe
Re: RFR [14/java.xml] 7148925: StAXSource causes exceptions to be thrown with certain wellformed XML instances
On 7/9/19 3:07 PM, Lance Andersen wrote: Hi Joe, Overall, this looks good. Thanks Lance! Please add the bug number to the top @bug in the test class. Added. I might have defined all of the elements in the DataProvider “xml” in separate String objects to make it easier to read, but no big deal and does not need changed prior to pushing. Thanks. I'm going to keep them this way then ;-) They are small enough. -Joe On Jul 9, 2019, at 5:17 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a fix for an Exception caused by StAXSource. The fix adds code that processes the prolog and misc content as specified by the XML specification, and removes the code that made incorrect assumption about XML document structure. https://bugs.openjdk.java.net/browse/JDK-7148925 http://cr.openjdk.java.net/~joehw/jdk14/7148925/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR [14/java.xml] 7148925: StAXSource causes exceptions to be thrown with certain wellformed XML instances
Please review a fix for an Exception caused by StAXSource. The fix adds code that processes the prolog and misc content as specified by the XML specification, and removes the code that made incorrect assumption about XML document structure. https://bugs.openjdk.java.net/browse/JDK-7148925 http://cr.openjdk.java.net/~joehw/jdk14/7148925/webrev/ Thanks, Joe
Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML
Thanks Lance! -Joe On 7/2/19, 10:22 AM, Lance Andersen wrote: Looks OK to me Joe On Jul 2, 2019, at 11:51 AM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Thanks Daniel. That test case is added. Best, Joe On 7/2/19, 1:21 AM, Daniel Fuchs wrote: Hi Joe, I haven't spotted anything obviously wrong. The content of "abcxyz" should be "abc & xyz", not " abc\n & \nxyz\n" as that before this fix. Maybe the test could have a test case for that specific example too? Just to make sure whitespaces in CDATA aren't eaten away? best regards, -- daniel On 01/07/2019 20:46, Joe Wang wrote: Please review a fix to xml pretty print. This is a regression introduced during the JDK 9 development. CDATA is marked up to be interpreted literally as textual data, in other words, it's still character data. The processor therefore shall treat it as text data. The content of "abcxyz" should be "abc & xyz", not "abc\n & \n xyz\n" as that before this fix. JBS: https://bugs.openjdk.java.net/browse/JDK-8223291 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/ <http://cr.openjdk.java.net/%7Ejoehw/jdk14/8223291/webrev/> Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML
Thanks Daniel! On 7/2/19, 9:16 AM, Daniel Fuchs wrote: Hi Joe, On 02/07/2019 17:51, Joe Wang wrote: Thanks Daniel. That test case is added. LGTM! best regards, -- daniel Best, Joe On 7/2/19, 1:21 AM, Daniel Fuchs wrote: Hi Joe, I haven't spotted anything obviously wrong. The content of "abcxyz" should be "abc & xyz", not "abc\n & \nxyz\n" as that before this fix. Maybe the test could have a test case for that specific example too? Just to make sure whitespaces in CDATA aren't eaten away? best regards, -- daniel On 01/07/2019 20:46, Joe Wang wrote: Please review a fix to xml pretty print. This is a regression introduced during the JDK 9 development. CDATA is marked up to be interpreted literally as textual data, in other words, it's still character data. The processor therefore shall treat it as text data. The content of "abcxyz" should be "abc & xyz", not "abc\n & \n xyz\n" as that before this fix. JBS: https://bugs.openjdk.java.net/browse/JDK-8223291 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/ Thanks, Joe
Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML
On 7/2/19, 1:23 AM, Daniel Fuchs wrote: Oh, and BTW is the change to PrettyPrintTest.java intended? (the removal of the line: 385 dbf.setValidating(true); seems to be the only change) Yes. The only effect of setting validating without an ErrorHandler was producing lots of warnings in the test result. It was therefore removed. Best, Joe best regards, -- daniel On 02/07/2019 10:21, Daniel Fuchs wrote: JBS: https://bugs.openjdk.java.net/browse/JDK-8223291 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/
Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML
Thanks Daniel. That test case is added. Best, Joe On 7/2/19, 1:21 AM, Daniel Fuchs wrote: Hi Joe, I haven't spotted anything obviously wrong. The content of "abcxyz" should be "abc & xyz", not "abc\n & \nxyz\n" as that before this fix. Maybe the test could have a test case for that specific example too? Just to make sure whitespaces in CDATA aren't eaten away? best regards, -- daniel On 01/07/2019 20:46, Joe Wang wrote: Please review a fix to xml pretty print. This is a regression introduced during the JDK 9 development. CDATA is marked up to be interpreted literally as textual data, in other words, it's still character data. The processor therefore shall treat it as text data. The content of "abcxyz" should be "abc & xyz", not "abc\n & \n xyz\n" as that before this fix. JBS: https://bugs.openjdk.java.net/browse/JDK-8223291 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/ Thanks, Joe
RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML
Please review a fix to xml pretty print. This is a regression introduced during the JDK 9 development. CDATA is marked up to be interpreted literally as textual data, in other words, it's still character data. The processor therefore shall treat it as text data. The content of "abcxyz" should be "abc & xyz", not "abc\n & \n xyz\n" as that before this fix. JBS: https://bugs.openjdk.java.net/browse/JDK-8223291 webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/ Thanks, Joe
Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1
On 6/24/19, 12:00 PM, Daniel Fuchs wrote: On 24/06/2019 18:24, Joe Wang wrote: Keeping the code in sync with the BCEL (not perfect) source is in our interest to produce smaller changeset in any future updates. Would you be okay with the current webrevs? Yes. This was just a remark that maybe the inconsistency should be reported upstream (in case they want to fix it sometime in the future). I wasn't suggesting to fix it downstream, or to postpone the patch until it's fixed. Sorry if this wasn't clear. Ah, ok. Thanks! :-) Best, Joe best regards, -- daniel
Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1
On 6/24/19, 2:23 AM, Daniel Fuchs wrote:
On 21/06/2019 21:20, Joe Wang wrote:
Full webrev (for the record)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/
A short version of webrev_02 that includes the only files mentioned
in this review:
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/
Thanks Joe!
The updated files look good.
Thumbs up!
Thanks!
Minor comment - that you might want to propagate upstream:
The hashCode impl is indeed inconsistent with equals. Looking at the
issue [1], by using Objects.equals, they were fixing a possible NPE.
Either case (the original or the use of Objects.equals) does not affect
java.xml's specific usage of BCEL. The original JAXP team used to be
part of Apache projects, but I myself hasn't been. While we would like
it to be perfect (in format, and in issues such as this), I think we
have to live with it, acknowledging it but knowing the edge case does
not affect our usage.
Keeping the code in sync with the BCEL (not perfect) source is in our
interest to produce smaller changeset in any future updates. Would you
be okay with the current webrevs?
[1] https://issues.apache.org/jira/browse/BCEL-297
Best regards,
Joe
@Override
-public boolean equals(final Object o1, final Object o2) {
+public boolean equals( final Object o1, final Object o2 ) {
final JavaClass THIS = (JavaClass) o1;
final JavaClass THAT = (JavaClass) o2;
-return THIS.getClassName().equals(THAT.getClassName());
+return Objects.equals(THIS.getClassName(),
THAT.getClassName());
}
+
@Override
-public int hashCode(final Object o) {
+public int hashCode( final Object o ) {
final JavaClass THIS = (JavaClass) o;
return THIS.getClassName().hashCode();
}
I've seen that idiom at a few places: equals was modified to
use Objects.equals(), but hashCode was not.
1. equals will throw if it is passed a null THIS or THAT (is
that intented?)
2. equals seems to think that getClassName() can return null,
whereas hashCode obviously expect that it will be non null.
I cannot say whether that's right or wrong, but I can smell something...
Maybe having a look at the issue that triggered the use of
Objects::equals could shed some light on this. It doesn't seem worse
than what was there before - it's just a bit odd.
best regards,
-- daniel
Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1
Thanks Lance!
Joe
On 6/21/19, 1:59 PM, Lance Andersen wrote:
The revised webrev looks fine Joe
On Jun 21, 2019, at 4:20 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote:
On 6/21/19, 11:59 AM, Daniel Fuchs wrote:
Hi Joe,
I promise, I will not grumble about the formatting and
weird white spaces [grumble grumble]...
Someone at BCEL needs to re-format the source with a modern IDE ;-)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html
<http://cr.openjdk.java.net/%7Ejoehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html>
Is the new import really needed here?
No, good catch.
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html
<http://cr.openjdk.java.net/%7Ejoehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html>
232 if (!dir.isDirectory()) {
I see use of SecuritySupport was dropped here and I think I agree,
as I don't see why isDirectory() would require it if mkdirs() doesn't.
I honestly don't remember why I added that in the previous update.
But you're right, and all test run proved it.
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html
<http://cr.openjdk.java.net/%7Ejoehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html>
Are the changes in toString() actually correct?
No, it's not. Good catch again! The change in the last update was correct
Otherwise I believe the rest looks mostly OK.
Thanks Daniel! Here's an updated webrev:
Full webrev (for the record)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/
<http://cr.openjdk.java.net/%7Ejoehw/jdk13/8224157/webrev_02/>
A short version of webrev_02 that includes the only files mentioned
in this review:
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/
I'm re-running all tests.
Best regards,
Joe
best regards,
-- daniel
On 20/06/2019 23:45, Joe Wang wrote:
Please review an update to BCEL 6.3.1. This changeset will go into
JDK13 if approved, 14 if not.
1. Format
The changeset looks big, the majority of the changes however
were only different in format (i.e. Const.java). Unlike previous
updates, I'm leaving the format as they are in the upstream source
so that they won't show up in future updates. The only change I
made were those that had extremely long lines.
2. Exclusions
Since the BCEL component is for xml transform only, several
classes, that were not in the JDK, were excluded. Among them,
JavaWrapper.java for example can be problematic as it may use an
user-specified classloader to load arbitrary classes. It and
related classes were therefore excluded.
3. Warnings
Warnings were the main reason for the changes made to the
original source. It has been done in the previous update. For this
update therefore, I only had to re-apply them after making copies
of the upstream source. Still, I updated the LastModified field to
indicate a modification to the original source.
4. Deprecated fields to private
Deprecated fields in the original source were changed to
private ones in previous update. The changes are inherited in this
update. Again, the LastModified fields are also updated.
5. Test
Since the update does not affect the usage of the BCEL
component, it is essential to pass all of the existing tests. I've
run the tests multiple times and noted that all of the XML
functional and unit tests passed, so were JCK XML tests. I've also
done a comparison between builds before and after applying the BCEL
update, and found no change in performance.
JBS: https://bugs.openjdk.java.net/browse/JDK-8224157
webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/
Thanks,
Joe
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: RFR (JDK 14/java.xml) 8224157: BCEL: update to version 6.3.1
Thanks Lance! Yes, JDK 14 it is, as Mark has asked us to retarget it to. Best, Joe On 6/21/19, 1:23 PM, Lance Andersen wrote: Hi Joe, Overall looks OK. Its unfortunate to loose the formatting but given it is not upstream, best to not have the additional work of going through this exercise each time. I have no additional changes to propose outside of what Daniel caught. Even though there are a large number of files, the updates/impact would be minimal if approved for JDFK 13. It might be easier though to just target JDK 14 given where we are in the release cycle. HTH Lance On Jun 20, 2019, at 6:45 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review an update to BCEL 6.3.1. This changeset will go into JDK13 if approved, 14 if not. 1. Format The changeset looks big, the majority of the changes however were only different in format (i.e. Const.java). Unlike previous updates, I'm leaving the format as they are in the upstream source so that they won't show up in future updates. The only change I made were those that had extremely long lines. 2. Exclusions Since the BCEL component is for xml transform only, several classes, that were not in the JDK, were excluded. Among them, JavaWrapper.java for example can be problematic as it may use an user-specified classloader to load arbitrary classes. It and related classes were therefore excluded. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. For this update therefore, I only had to re-apply them after making copies of the upstream source. Still, I updated the LastModified field to indicate a modification to the original source. 4. Deprecated fields to private Deprecated fields in the original source were changed to private ones in previous update. The changes are inherited in this update. Again, the LastModified fields are also updated. 5. Test Since the update does not affect the usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. I've also done a comparison between builds before and after applying the BCEL update, and found no change in performance. JBS: https://bugs.openjdk.java.net/browse/JDK-8224157 webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/ <http://cr.openjdk.java.net/%7Ejoehw/jdk13/8224157/webrev/> Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1
On 6/21/19, 11:59 AM, Daniel Fuchs wrote:
Hi Joe,
I promise, I will not grumble about the formatting and
weird white spaces [grumble grumble]...
Someone at BCEL needs to re-format the source with a modern IDE ;-)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html
Is the new import really needed here?
No, good catch.
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html
232 if (!dir.isDirectory()) {
I see use of SecuritySupport was dropped here and I think I agree,
as I don't see why isDirectory() would require it if mkdirs() doesn't.
I honestly don't remember why I added that in the previous update. But
you're right, and all test run proved it.
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html
Are the changes in toString() actually correct?
No, it's not. Good catch again! The change in the last update was correct
Otherwise I believe the rest looks mostly OK.
Thanks Daniel! Here's an updated webrev:
Full webrev (for the record)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/
A short version of webrev_02 that includes the only files mentioned in
this review:
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/
I'm re-running all tests.
Best regards,
Joe
best regards,
-- daniel
On 20/06/2019 23:45, Joe Wang wrote:
Please review an update to BCEL 6.3.1. This changeset will go into
JDK13 if approved, 14 if not.
1. Format
The changeset looks big, the majority of the changes however
were only different in format (i.e. Const.java). Unlike previous
updates, I'm leaving the format as they are in the upstream source so
that they won't show up in future updates. The only change I made
were those that had extremely long lines.
2. Exclusions
Since the BCEL component is for xml transform only, several
classes, that were not in the JDK, were excluded. Among them,
JavaWrapper.java for example can be problematic as it may use an
user-specified classloader to load arbitrary classes. It and related
classes were therefore excluded.
3. Warnings
Warnings were the main reason for the changes made to the
original source. It has been done in the previous update. For this
update therefore, I only had to re-apply them after making copies of
the upstream source. Still, I updated the LastModified field to
indicate a modification to the original source.
4. Deprecated fields to private
Deprecated fields in the original source were changed to private
ones in previous update. The changes are inherited in this update.
Again, the LastModified fields are also updated.
5. Test
Since the update does not affect the usage of the BCEL
component, it is essential to pass all of the existing tests. I've
run the tests multiple times and noted that all of the XML functional
and unit tests passed, so were JCK XML tests. I've also done a
comparison between builds before and after applying the BCEL update,
and found no change in performance.
JBS: https://bugs.openjdk.java.net/browse/JDK-8224157
webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/
Thanks,
Joe
RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1
Please review an update to BCEL 6.3.1. This changeset will go into JDK13 if approved, 14 if not. 1. Format The changeset looks big, the majority of the changes however were only different in format (i.e. Const.java). Unlike previous updates, I'm leaving the format as they are in the upstream source so that they won't show up in future updates. The only change I made were those that had extremely long lines. 2. Exclusions Since the BCEL component is for xml transform only, several classes, that were not in the JDK, were excluded. Among them, JavaWrapper.java for example can be problematic as it may use an user-specified classloader to load arbitrary classes. It and related classes were therefore excluded. 3. Warnings Warnings were the main reason for the changes made to the original source. It has been done in the previous update. For this update therefore, I only had to re-apply them after making copies of the upstream source. Still, I updated the LastModified field to indicate a modification to the original source. 4. Deprecated fields to private Deprecated fields in the original source were changed to private ones in previous update. The changes are inherited in this update. Again, the LastModified fields are also updated. 5. Test Since the update does not affect the usage of the BCEL component, it is essential to pass all of the existing tests. I've run the tests multiple times and noted that all of the XML functional and unit tests passed, so were JCK XML tests. I've also done a comparison between builds before and after applying the BCEL update, and found no change in performance. JBS: https://bugs.openjdk.java.net/browse/JDK-8224157 webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/ Thanks, Joe
Re: RFR (JDK 13/java.xml) 8223658: Performance regression of XML.validation in 13-b19
Thanks Lance! Pushed. Let's hope the performance will be back to normal in the next build. Best, Joe On 5/24/19, 10:18 AM, Lance Andersen wrote: +1 On May 24, 2019, at 1:11 PM, Joe Wang <mailto:huizhe.w...@oracle.com>> wrote: Please review a fix to the performance regression introduced by Xerces update. The performance benchmark gives weight to small XML files. Large chunk sizes are therefore not helpful. Performance tests with the following patch showed that the results are on par with that for JDK 13 b18. https://bugs.openjdk.java.net/browse/JDK-8223658 http://cr.openjdk.java.net/~joehw/jdk13/8223658/webrev/ Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
RFR (JDK 13/java.xml) 8223658: Performance regression of XML.validation in 13-b19
Please review a fix to the performance regression introduced by Xerces update. The performance benchmark gives weight to small XML files. Large chunk sizes are therefore not helpful. Performance tests with the following patch showed that the results are on par with that for JDK 13 b18. https://bugs.openjdk.java.net/browse/JDK-8223658 http://cr.openjdk.java.net/~joehw/jdk13/8223658/webrev/ Thanks, Joe
