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

Tue Dec 4 11:33:27 UTC 2018

Hello Magnus.

IBM33722 should be in sun.nio.cs.ext package on jdk.charset module
Because it's not used for default encoding on AIX platform.

In case of template file, build tool checks 
make/data/charsetmapping/stdcs-aix file for this case.
If class name is there, it will be migrated to sun.nio.cs package.
About IBM33722,
If IBM33722 is moved to sun.nio.cs package also,
"import sun.nio.cs.*;" is no need on IBM33722.java
If IBM33722 is not in stdcs-aix,
"import sun.nio.cs.*;" is still required on IBM33722.java

Thanks,
Ichiroh Takiguchi

On 2018-12-04 19:42, Magnus Ihse Bursie wrote:
> On 2018-12-04 04:30, 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'm not sure I'm fully following the template intricacies here, but
> would this not be solved if IBM33722.java was made a template as well?
> Then you could do
> import $PACKAGE$. SimpleEUCEncoder
> Or so I'd think.
> 
> /Magnus
>> 
>>> 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
>>>>> 
>>>> 
>> 