Hi Ichiroh,
Looks fine.
I can sponsor that for you. Tomorrow, though, to allow time for any
other comments.
Thanks, Roger
On 12/04/2018 10:21 AM, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.04/
Thanks,
Ichiroh Takiguchi
On 2018-12-04 23:36, Roger Riggs wrote:
Hi Ichiroh,
On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers
it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext
package
I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I understand, it is because IBM33722 may or may not be in the same
package as SimpleEUCEncoder.
It is SimpleEUCEncoder that may be in a different package, not IBM33722.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
I'll changed.
226-227: add a space before "{" to have the same style as line 210.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Yes, it's no need.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/
Thanks, looks fine.
Regards, Roger
Thanks,
Ichiroh Takiguchi
On 2018-12-04 05:50, Roger Riggs wrote:
Hi Ichiroh,
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Regards, Roger
On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:
On 2018-11-30 10:49, Ichiroh Takiguchi wrote:
Hello.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review
it too.
/Magnus
I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)
On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.
Without fix
$ jshell
| Welcome to JShell -- Version 12-ea
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964
jshell> /exit
| Goodbye
$
======
With fix
======
$ jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
======
On AIX platform
IBM964 is moved to java.base module from jdk.charset module.
======
$ LANG=zh_TW jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
======
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-11-29 22:39, Ichiroh Takiguchi wrote:
Hello Alan & Magnus.
Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.
I think I should not put 2 kind of modification.
For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)
I'll submit code review again.
Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix
I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java
My build machine is not so fast, after test is done.
I'll post new code.
Thanks,
Ichiroh Takiguchi
On 2018-11-28 19:10, Magnus Ihse Bursie wrote:
On 2018-11-28 10:36, Alan Bateman wrote:
On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character
sets in the build in general. :-( I'd really like to
modernize it. I have a, slightly fuzzy, laundry list of
things I want to fix from a build perspective, but I'm not
sure of what "external" requirements are coming from AIX and
the general core-libs agenda regarding character sets in
general.
I think there is a good opportunity to solve many problems at
the same time here, as long as everyone agrees on what is the
preferred outcome.
The support in the build to configure the charsets to include
in java.base on each platform has been working well. Charsets
that aren't in java.base go into the jdk.charsets service
provider module and that has been working well too. From the
result point of view, perhaps, but definitely not from the
build perspective. ;-) But yes, I understand this is
functionality that should be kept.
One thing that we lack is some way to add charsets for
specific platforms and this comes up with the IBM patches
where they are looking to adding several additional IBM
charsets. One starting point that we've touched on in several
threads here is dropping the EBCDIC charsets from the main
stream builds. Going there will need build support.
So build support for trivially adding specific charsets to
specific
platforms? Both to java.base (for AIX) and jdk.charsets, I
presume,
then?
Can you expand on the issue of dropping ebcdic? What's the problem
that needs build support?
/Magnus
-Alan