RE: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Correcting typo for release. From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Hi All, Please review the backport of the following bug fixes to jdk11u-dev: HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/ These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace Regards, Deepak
Hi Deepak, Please follow the jdk-updates approval process: https://openjdk.java.net/projects/jdk-updates/approval.html You need to put appropriate tags at the issues, explain why they are needed, how the changes apply (if changes do not apply cleanly, ask for RFR -- is this what this thread is? -- and explain what was done to adapt changes to 11u), what testing is done, etc. -Aleksey On 2/19/19 3:15 PM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
On 19/02/2019 14:25, Aleksey Shipilev wrote:
Hi Deepak,
Please follow the jdk-updates approval process: https://openjdk.java.net/projects/jdk-updates/approval.html
You need to put appropriate tags at the issues, explain why they are needed, how the changes apply (if changes do not apply cleanly, ask for RFR -- is this what this thread is? -- and explain what was done to adapt changes to 11u), what testing is done, etc.
I read Deepak's mails as code review requests. The context is issues related to support for external standards. There are MRs of both JSR 337 and JSR 384 in progress. Iris sent a good summary to jdk-update-dev in December [1]. Once the changes are reviewed then I assume Deepak will figure out the process/tagging to get the changes into the update repos but I don't think he's there yet. -Alan [1] https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2018-December/000308...
Deepak, changes look fine to me. Some minor comments on formatting : make a space after all "//" comments e.g. + //isJavaIdentifierStart strictly conforms to code points assigned + //in Unicode 10.0. Since code point {32FF} is not from Unicode 10.0, + //return false. 33 //New code point(Japanese Era Square character) not present in Unicode 10.0 34 private static final int newCodePoint = 0x32FF; 65 //Since Character.isJavaIdentifierPart(int) strictly conforms to 66 //character information from version 10.0 of the Unicode Standard, 67 //check if code point is new code point. If the code point is new 68 //code point, value of variable expected is considered false. typo in comment : make/data/characterdata/CharacterData00.java.template boolean isJavaIdentifierPart(int ch) { + //isJavaIdentifierStart strictly conforms to code points assigned + //in Unicode 10.0. Since code point {32FF} is not from Unicode 10.0, regards, Sean. On 19/02/2019 14:15, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
Hi Deepak, Here are my comments to the webrev (other than what Sean pointed out): TestIsJavaIdentifierMethods.java - @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit". - Line 34: "newCodePoint" does not represent the era character, as "new" is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"? - Line 67,68,(...all the comments): Reflect the above change to the comments. - Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct. It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions. - Line 104: The test case returns "void", what does this "Expected results" mean? - Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int): isLetter(codePoint) returns true getType(codePoint) returns LETTER_NUMBER the referenced character is a currency symbol (such as '$') the referenced character is a connecting punctuation character (such as '_'). - Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart(). Naoto On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
Thanks Naoto San and Sean for review. I have incorporate all the comments. Please find below updated version of webrev :- http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/ Regards, Deepak -----Original Message----- From: Naoto Sato Sent: Tuesday, February 19, 2019 10:23 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Hi Deepak, Here are my comments to the webrev (other than what Sean pointed out): TestIsJavaIdentifierMethods.java - @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit". - Line 34: "newCodePoint" does not represent the era character, as "new" is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"? - Line 67,68,(...all the comments): Reflect the above change to the comments. - Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct. It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions. - Line 104: The test case returns "void", what does this "Expected results" mean? - Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int): isLetter(codePoint) returns true getType(codePoint) returns LETTER_NUMBER the referenced character is a currency symbol (such as '$') the referenced character is a connecting punctuation character (such as '_'). - Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart(). Naoto On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
Hi Deepak, I see the following comment is not addressed yet:
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto On 2/20/19 8:53 AM, Deepak Kejriwal wrote:
Thanks Naoto San and Sean for review. I have incorporate all the comments. Please find below updated version of webrev :-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/
Regards, Deepak
-----Original Message----- From: Naoto Sato Sent: Tuesday, February 19, 2019 10:23 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
Here are my comments to the webrev (other than what Sean pointed out):
TestIsJavaIdentifierMethods.java
- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit".
- Line 34: "newCodePoint" does not represent the era character, as "new" is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"?
- Line 67,68,(...all the comments): Reflect the above change to the comments.
- Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct. It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions.
- Line 104: The test case returns "void", what does this "Expected results" mean?
- Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int):
isLetter(codePoint) returns true getType(codePoint) returns LETTER_NUMBER the referenced character is a currency symbol (such as '$') the referenced character is a connecting punctuation character (such as '_').
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
Hi Naoto, Corrected the exception message. Please find below updated version of webrev:- http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/ Thanks for review. Regards, Deepak -----Original Message----- From: Naoto Sato Sent: Wednesday, February 20, 2019 11:06 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; Sean Coffey <sean.coffey@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Hi Deepak, I see the following comment is not addressed yet:
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto On 2/20/19 8:53 AM, Deepak Kejriwal wrote:
Thanks Naoto San and Sean for review. I have incorporate all the comments. Please find below updated version of webrev :-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/
Regards, Deepak
-----Original Message----- From: Naoto Sato Sent: Tuesday, February 19, 2019 10:23 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
Here are my comments to the webrev (other than what Sean pointed out):
TestIsJavaIdentifierMethods.java
- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit".
- Line 34: "newCodePoint" does not represent the era character, as "new" is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"?
- Line 67,68,(...all the comments): Reflect the above change to the comments.
- Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct. It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions.
- Line 104: The test case returns "void", what does this "Expected results" mean?
- Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int):
isLetter(codePoint) returns true getType(codePoint) returns LETTER_NUMBER the referenced character is a currency symbol (such as '$') the referenced character is a connecting punctuation character (such as '_').
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
Deepak, this exception message in new test still needs correction:
166 "Character.isLetter(int) failed for codepoint " 167 + Integer.toHexString(cp)); As an aside, there's probably no need for such specific exception messages in a test case. It's error prone (but you've come this far now)
regards, Sean. On 21/02/2019 08:26, Deepak Kejriwal wrote:
Hi Naoto,
Corrected the exception message. Please find below updated version of webrev:- http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/
Thanks for review.
Regards, Deepak
-----Original Message----- From: Naoto Sato Sent: Wednesday, February 20, 2019 11:06 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; Sean Coffey <sean.coffey@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
I see the following comment is not addressed yet:
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/20/19 8:53 AM, Deepak Kejriwal wrote:
Thanks Naoto San and Sean for review. I have incorporate all the comments. Please find below updated version of webrev :-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/
Regards, Deepak
-----Original Message----- From: Naoto Sato Sent: Tuesday, February 19, 2019 10:23 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
Here are my comments to the webrev (other than what Sean pointed out):
TestIsJavaIdentifierMethods.java
- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit".
- Line 34: "newCodePoint" does not represent the era character, as "new" is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"?
- Line 67,68,(...all the comments): Reflect the above change to the comments.
- Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct. It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions.
- Line 104: The test case returns "void", what does this "Expected results" mean?
- Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int):
isLetter(codePoint) returns true getType(codePoint) returns LETTER_NUMBER the referenced character is a currency symbol (such as '$') the referenced character is a connecting punctuation character (such as '_').
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal <deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' <core-libs-dev@openjdk.java.net>; 'jdk-updates-dev@openjdk.java.net' <jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards, Deepak
Hi Sean, The webrev I shared was not correct. I have corrected the webrev.02 now. Please check now:- http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/ Regards, Deepak From: Seán Coffey Sent: Thursday, February 21, 2019 2:11 PM To: Deepak Kejriwal <deepak.kejriwal@oracle.com>; Naoto Sato <naoto.sato@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Deepak, this exception message in new test still needs correction: 166 "Character.isLetter(int) failed for codepoint " 167 + Integer.toHexString(cp)); As an aside, there's probably no need for such specific exception messages in a test case. It's error prone (but you've come this far now) regards, Sean. On 21/02/2019 08:26, Deepak Kejriwal wrote: Hi Naoto, Corrected the exception message. Please find below updated version of webrev:- http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/ Thanks for review. Regards, Deepak -----Original Message----- From: Naoto Sato Sent: Wednesday, February 20, 2019 11:06 PM To: Deepak Kejriwal HYPERLINK "mailto:deepak.kejriwal@oracle.com"<deepak.kejriwal@oracle.com>; Sean Coffey HYPERLINK "mailto:sean.coffey@oracle.com"<sean.coffey@oracle.com>; core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<core-libs-dev@openjdk.java.net>; HYPERLINK "mailto:jdk-updates-dev@openjdk.java.net"jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Hi Deepak, I see the following comment is not addressed yet:
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart(). Naoto On 2/20/19 8:53 AM, Deepak Kejriwal wrote:
Thanks Naoto San and Sean for review. I have incorporate all the comments. Please find below updated version of webrev :- http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/ Regards, Deepak -----Original Message----- From: Naoto Sato Sent: Tuesday, February 19, 2019 10:23 PM To: Deepak Kejriwal HYPERLINK "mailto:deepak.kejriwal@oracle.com"<deepak.kejriwal@oracle.com>; core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<core-libs-dev@openjdk.java.net>; HYPERLINK "mailto:jdk-updates-dev@openjdk.java.net"jdk-updates-dev@openjdk.java.net Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Hi Deepak, Here are my comments to the webrev (other than what Sean pointed out): TestIsJavaIdentifierMethods.java - @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit". - Line 34: "newCodePoint" does not represent the era character, as "new" is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"? - Line 67,68,(...all the comments): Reflect the above change to the comments. - Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct. It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions. - Line 104: The test case returns "void", what does this "Expected results" mean? - Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int): isLetter(codePoint) returns true getType(codePoint) returns LETTER_NUMBER the referenced character is a currency symbol (such as '$') the referenced character is a connecting punctuation character (such as '_'). - Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart(). Naoto On 2/19/19 6:15 AM, Deepak Kejriwal wrote: Correcting typo for release. From: Deepak Kejriwal HYPERLINK "mailto:deepak.kejriwal@oracle.com"<deepak.kejriwal@oracle.com> Sent: Tuesday, February 19, 2019 7:42 PM To: 'core-libs-dev' HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<core-libs-dev@openjdk.java.net>; 'HYPERLINK "mailto:jdk-updates-dev@openjdk.java.net"jdk-updates-dev@openjdk.java.net' HYPERLINK "mailto:jdk-updates-dev@openjdk.java.net"<jdk-updates-dev@openjdk.java.net> Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915 Hi All, Please review the backport of the following bug fixes to jdk11u-dev: HYPERLINK HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120""https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add test cases for lenient Japanese era parsing HYPERLINK HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398""https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : Square character support for the Japanese new era HYPERLINK HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915""https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points Webrev: http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/ These code changes are made possible thanks to specification change already pushed: http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace Regards, Deepak
Thanks. Looks good to me. regards, Sean. On 21/02/2019 09:10, Deepak Kejriwal wrote:
Hi Sean,
The webrev I shared was not correct. I have corrected the webrev.02 now. Please check now:-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/
Regards,
Deepak
*From:*Seán Coffey *Sent:* Thursday, February 21, 2019 2:11 PM *To:* Deepak Kejriwal <deepak.kejriwal@oracle.com>; Naoto Sato <naoto.sato@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net *Subject:* Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Deepak,
this exception message in new test still needs correction:
166 "Character.isLetter(int) failed for codepoint "
167 + Integer.toHexString(cp));
As an aside, there's probably no need for such specific exception messages in a test case. It's error prone (but you've come this far now)
regards, Sean.
On 21/02/2019 08:26, Deepak Kejriwal wrote:
Hi Naoto,
Corrected the exception message. Please find below updated version of webrev:-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/
Thanks for review.
Regards,
Deepak
-----Original Message-----
From: Naoto Sato
Sent: Wednesday, February 20, 2019 11:06 PM
To: Deepak Kejriwal<deepak.kejriwal@oracle.com> <mailto:deepak.kejriwal@oracle.com>; Sean Coffey<sean.coffey@oracle.com> <mailto:sean.coffey@oracle.com>; core-libs-dev<core-libs-dev@openjdk.java.net> <mailto:core-libs-dev@openjdk.java.net>;jdk-updates-dev@openjdk.java.net <mailto:jdk-updates-dev@openjdk.java.net>
Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
I see the following comment is not addressed yet:
> - Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/20/19 8:53 AM, Deepak Kejriwal wrote:
Thanks Naoto San and Sean for review. I have incorporate all the
comments. Please find below updated version of webrev :-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/
Regards,
Deepak
-----Original Message-----
From: Naoto Sato
Sent: Tuesday, February 19, 2019 10:23 PM
To: Deepak Kejriwal<deepak.kejriwal@oracle.com> <mailto:deepak.kejriwal@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net> <mailto:core-libs-dev@openjdk.java.net>;jdk-updates-dev@openjdk.java.net <mailto:jdk-updates-dev@openjdk.java.net>
Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
Here are my comments to the webrev (other than what Sean pointed out):
TestIsJavaIdentifierMethods.java
- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit".
- Line 34: "newCodePoint" does not represent the era character, as "new"
is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"?
- Line 67,68,(...all the comments): Reflect the above change to the comments.
- Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct.
It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions.
- Line 104: The test case returns "void", what does this "Expected results" mean?
- Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int):
isLetter(codePoint) returns true
getType(codePoint) returns LETTER_NUMBER
the referenced character is a currency symbol (such as '$')
the referenced character is a connecting punctuation character (such as '_').
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal<deepak.kejriwal@oracle.com> <mailto:deepak.kejriwal@oracle.com>
Sent: Tuesday, February 19, 2019 7:42 PM
To: 'core-libs-dev'<core-libs-dev@openjdk.java.net> <mailto:core-libs-dev@openjdk.java.net>;
'jdk-updates-dev@openjdk.java.net <mailto:jdk-updates-dev@openjdk.java.net>'<jdk-updates-dev@openjdk.java.net> <mailto:jdk-updates-dev@openjdk.java.net>
Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8206120" <https://bugs.openjdk.java.net/browse/JDK-8206120>JDK-8206120 : Add
test cases for lenient Japanese era parsing HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8211398" <https://bugs.openjdk.java.net/browse/JDK-8211398>JDK-8211398 :
Square character support for the Japanese new era HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8218915" <https://bugs.openjdk.java.net/browse/JDK-8218915>JDK-8218915 :
Change isJavaIdentifierStart and isJavaIdentifierPart to handle new
code points
Webrev:
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed:
http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards,
Deepak
+1 Please follow the appropriate process to push the changeset. Naoto On 2/21/19 2:24 AM, Seán Coffey wrote:
Thanks. Looks good to me.
regards, Sean.
On 21/02/2019 09:10, Deepak Kejriwal wrote:
Hi Sean,
The webrev I shared was not correct. I have corrected the webrev.02 now. Please check now:-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/
Regards,
Deepak
*From:*Seán Coffey *Sent:* Thursday, February 21, 2019 2:11 PM *To:* Deepak Kejriwal <deepak.kejriwal@oracle.com>; Naoto Sato <naoto.sato@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; jdk-updates-dev@openjdk.java.net *Subject:* Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Deepak,
this exception message in new test still needs correction:
166 "Character.isLetter(int) failed for codepoint "
167 + Integer.toHexString(cp));
As an aside, there's probably no need for such specific exception messages in a test case. It's error prone (but you've come this far now)
regards, Sean.
On 21/02/2019 08:26, Deepak Kejriwal wrote:
Hi Naoto,
Corrected the exception message. Please find below updated version of webrev:-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/
Thanks for review.
Regards,
Deepak
-----Original Message-----
From: Naoto Sato
Sent: Wednesday, February 20, 2019 11:06 PM
To: Deepak Kejriwal<deepak.kejriwal@oracle.com> <mailto:deepak.kejriwal@oracle.com>; Sean Coffey<sean.coffey@oracle.com> <mailto:sean.coffey@oracle.com>; core-libs-dev<core-libs-dev@openjdk.java.net> <mailto:core-libs-dev@openjdk.java.net>;jdk-updates-dev@openjdk.java.net <mailto:jdk-updates-dev@openjdk.java.net>
Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
I see the following comment is not addressed yet:
> - Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/20/19 8:53 AM, Deepak Kejriwal wrote:
Thanks Naoto San and Sean for review. I have incorporate all the
comments. Please find below updated version of webrev :-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/
Regards,
Deepak
-----Original Message-----
From: Naoto Sato
Sent: Tuesday, February 19, 2019 10:23 PM
To: Deepak Kejriwal<deepak.kejriwal@oracle.com> <mailto:deepak.kejriwal@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net> <mailto:core-libs-dev@openjdk.java.net>;jdk-updates-dev@openjdk.java.net <mailto:jdk-updates-dev@openjdk.java.net>
Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi Deepak,
Here are my comments to the webrev (other than what Sean pointed out):
TestIsJavaIdentifierMethods.java
- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> "isJavaLetterOrDigit".
- Line 34: "newCodePoint" does not represent the era character, as "new"
is subjective. It will become moot in the year 2020. How about "JAPANESE_ERA_CODEPOINT"?
- Line 67,68,(...all the comments): Reflect the above change to the comments.
- Line 103: "All Unicode chars (0x0000..0xFFFF)" does not sound correct.
It may be "All Unicode code points in the BMP (0x0000..0xFFFF), and remove extra period at the end. This applies to other method descriptions.
- Line 104: The test case returns "void", what does this "Expected results" mean?
- Line 140-142,174-176: The condition statement in the document is different from JDK11's javadoc. In the API doc, it is (in case of int):
isLetter(codePoint) returns true
getType(codePoint) returns LETTER_NUMBER
the referenced character is a currency symbol (such as '$')
the referenced character is a connecting punctuation character (such as '_').
- Line 163,198: Exception messages are incorrect. they are for isJavaIdentifierStart().
Naoto
On 2/19/19 6:15 AM, Deepak Kejriwal wrote:
Correcting typo for release.
From: Deepak Kejriwal<deepak.kejriwal@oracle.com> <mailto:deepak.kejriwal@oracle.com>
Sent: Tuesday, February 19, 2019 7:42 PM
To: 'core-libs-dev'<core-libs-dev@openjdk.java.net> <mailto:core-libs-dev@openjdk.java.net>;
'jdk-updates-dev@openjdk.java.net <mailto:jdk-updates-dev@openjdk.java.net>'<jdk-updates-dev@openjdk.java.net> <mailto:jdk-updates-dev@openjdk.java.net>
Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915
Hi All,
Please review the backport of the following bug fixes to jdk11u-dev:
HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8206120" <https://bugs.openjdk.java.net/browse/JDK-8206120>JDK-8206120 : Add
test cases for lenient Japanese era parsing HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8211398" <https://bugs.openjdk.java.net/browse/JDK-8211398>JDK-8211398 :
Square character support for the Japanese new era HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8218915" <https://bugs.openjdk.java.net/browse/JDK-8218915>JDK-8218915 :
Change isJavaIdentifierStart and isJavaIdentifierPart to handle new
code points
Webrev:
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/
These code changes are made possible thanks to specification change already pushed:
http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace
Regards,
Deepak
participants (6)
-
Alan Bateman
-
Aleksey Shipilev
-
Deepak Kejriwal
-
Naoto Sato
-
naoto.sato＠oracle.com
-
Seán Coffey