On Wed, 27 Apr 2022 17:20:11 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Ichiroh Takiguchi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285517: System.getenv() returns unexpected value if environment variable
>> has non ASCII character
>
> src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 68:
>
>> 66: private static final Map<String,String> theUnmodifiableEnvironment;
>> 67: static final int MIN_NAME_LENGTH = 0;
>> 68: static final Charset cs =
>> Charset.forName(StaticProperty.nativeEncoding(),
>
> cs should have an all-CAPS name to make it clear its a static value
> throughout the implementation.
> And `private` too.
Change to "private static final"
> test/jdk/java/lang/System/i18nEnvArg.java line 41:
>
>> 39:
>> 40: public class i18nEnvArg {
>> 41: final static String text = "\u6F22\u5B57";
>
> Can this be renamed to indicate it is the string under test? like KANJI_TEXT
> or EUC_JP_TEXT or similar.
Use EUC_JP_TEXT
> test/jdk/java/lang/System/i18nEnvArg.java line 49:
>
>> 47: final static int maxSize = 4096;
>> 48:
>> 49: public static void main(String[] args) throws Exception {
>
> There are 3 main()'s in this test; it would be useful to add a comment
> indicating the purpose of each.
Add comments
> test/jdk/java/lang/System/i18nEnvArg.java line 60:
>
>> 58: Process p = pb.start();
>> 59: InputStream is = p.getInputStream();
>> 60: byte[] ba = new byte[maxSize];
>
> You can use `InputStream.readAllBytes()` here to return a byte buffer.
>
> Its not clear why all the bytes are read? I assume its only for a possible
> error from the child.
Use InputStream.readAllBytes()
> test/jdk/java/lang/System/i18nEnvArg.java line 128:
>
>> 126: Method enviorn_mid = cls.getDeclaredMethod("environ");
>> 127: enviorn_mid.setAccessible(true);
>> 128: byte[][] environ = (byte[][]) enviorn_mid.invoke(null,
>
> typo: "enviorn_mid" -> "environ_mid".
Fix typo
> test/jdk/java/lang/System/i18nEnvArg.java line 138:
>
>> 136: bb.put(environ[i+1]);
>> 137: byte[] envb = Arrays.copyOf(ba, bb.position());
>> 138: if (Arrays.equals(kanji, envb)) return;
>
> It seems odd to exit the loop on the first match.
>
> I think AIX always has environment variables not set by the caller.
On this testing, use 2 environment variables:
* LANG=ja_JP.eucjp
* \u6F22\u5B57=\u6F22\u5B57
If the other environment variable is defined, it's printed.
> test/jdk/java/lang/System/i18nEnvArg.java line 146:
>
>> 144: sb.append((char)b);
>> 145: } else {
>> 146: sb.append(String.format("\\x%02X", (int)b &
>> 0xFF));
>
> The methods of java.util.HexFormat may be useful here.
> It seems that the "5C" would fit into this "else" clause and not need a
> separate statement.
Use HexFormat class
-------------
PR: https://git.openjdk.java.net/jdk/pull/8378
