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

Tue Dec 4 15:21:12 UTC 2018

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