RFR: 8212794 IBM-964 is required for AIX default charset

Tue Dec 4 03:30:04 UTC 2018

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.

> 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.

>   - 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,
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
>>> 
>> 