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