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

Tue Dec 4 16:07:22 UTC 2018

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