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

Tue Dec 4 14:36:41 UTC 2018

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