Hi Serguei, Thanks for your review and advice.
I modified these. Could you review this again, please? Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.05/ Regards, Chihiro 2020年2月25日(火) 3:36 serguei.spit...@oracle.com <serguei.spit...@oracle.com>: > Hi Chihiro, > > Thank you for taking care about this issue! > > It looks good to me. > Just a couple of minor comments. > > > http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.04/src/java.base/share/classes/jdk/internal/vm/VMSupport.java.frames.html > > 88 private static String toEscapeSpecialChar(String theString) { > > I'd suggest to use replace parameter name "theString" with "source" as you > have below: > > 92 private static String toEscapeSpace(String source) { > 96 private static String toISO88591(String source) throws > CharacterCodingException { > > > 98 var byteBuf = ByteBuffer.allocate(charBuf.length() * 6); // 6 is > 2 bytes for '\\u' as String and 4 bytes for code point. > > I'd suggest to put the comment before as a separate line. > > > 107 byteBuf.put(String.format("\\u%04X", (int) > charBuf.get()).getBytes()); > > Space is not needed after the cast in: (int) charBuf.get(). > > > > http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.04/test/jdk/sun/tools/jcmd/TestVM.java.html > > 2 * Copyright (c) 2020, 2020, Oracle and/or its affiliates. All rights > reserved. > > Could you, please, replace: 2020, 2020 => "2020? > I don't think two numbers are needed for the same year. > > > Thanks, > Serguei > > > On 2/22/20 07:37, Chihiro Ito wrote: > > Hi Yasumasa, > > Thank you for your reviews so many times. > How is this fix? > Could you review this again, please? > > Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.04/ > > Regards, > Chihiro > > 2020年2月22日(土) 21:53 Yasumasa Suenaga <suen...@oss.nttdata.com>: > >> Hi Chihiro, >> >> >> - My proposal is not enough, so you should refine as below. >> - Exception types in saveConvert() should be limited. Please do >> not use `throws Exception`. >> - I guess you use try-catch statement in >> serializePropertiesToByteArray due to above checked exception. >> It should be throw runtime exception when an exception occurs. >> - Capacity of byteBuf (charBuf.length() * 5) should be >> (charBuf.length() * 6) >> because non 8859-1 chars would be "\uxxxx" (6 chars). >> Also please leave comment for it because a maintainer might not >> understand the meaning of multiplying 6 in future. >> >> - `output.shouldNotContain("C:\\:\\\\");` in testcase is correct? >> I guess you want to check "C\\:\\\\" is not contained. >> >> - To check '\n', you can use Platform::isWindows as below: >> output.shouldContain(Platform.isWindows() ? >> "line.separator=\\r\\n" : "lineseparator=\\n"); >> >> >> Yasumasa >> >> >> On 2020/02/22 19:23, Chihiro Ito wrote: >> > Hi Yasumasa, >> > >> > The line separator is not modified because it depends on the >> environment, but the others have been modified. >> > >> > Could you review this again? >> > >> > Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.03/ >> > >> > Regards, >> > Chihiro >> > >> > 2020年2月22日(土) 12:32 Yasumasa Suenaga <suen...@oss.nttdata.com <mailto: >> suen...@oss.nttdata.com>>: >> > >> > Hi Chihiro, >> > >> > Thank you for updating the webrev. >> > >> > >> > - You use BufferedWriter to create the output, however I think >> it would be more simply if you use PrintWriter. >> > >> > - Your change would work incorrectly when system property >> contains mixture of ascii and non-ascii. >> > You can see it with "-Dmixture=aあi". It would be converted to >> "a\u0061\u3042", it should be "a\u3042i". >> > >> > - Currently key value which contains space char, it would be >> escaped, but your change does not do so. >> > You can see it with "-D"space space=blank blank"". >> > >> > - You should not use String::trim to create String from >> ByteBuffer because property value might be contain blank in its tail. >> > You might use ByteBuffer::slice or part of ByteBuffer::array >> for it. >> > >> > - Did you try to use escaped chars in jtreg testcase? I guess >> you can set multibytes chars (e.g. CJK chars) with "\u". >> > In case of mixture of Japanese (Hiragana) and ASCII chars, >> you can embed "-Dmixture=a\u3042i" to testcase. (I'm not sure that...) >> > >> > - In test case, I recommend you to evaluate entire of line. >> > For example, if you want to check line.separator, you should >> evaluate as below: >> > output.shouldContain("line.separator=\\n"); >> > >> > >> > Thanks, >> > >> > Yasumasa >> > >> > >> > On 2020/02/22 0:44, Chihiro Ito wrote: >> > > Hi Yasumasa, >> > > >> > > Thank you for your advice. >> > > >> > > I decided not to use regular expressions. because of the number >> of \is confusing. >> > > I stopped using codePointAt() and used CharsetEncoder to work >> with ISO 8859 -1. >> > > I added some environment variables to the test. However, >> environment variables that contain multi bytes or spaces are not included >> because jtreg does not support them. >> > > >> > > Could you review this again, please? >> > > >> > > Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.02/ >> > > >> > > Regards, >> > > Chihiro >> > > >> > > 2020年2月20日(木) 22:39 Yasumasa Suenaga <suen...@oss.nttdata.com >> <mailto:suen...@oss.nttdata.com> <mailto:suen...@oss.nttdata.com <mailto: >> suen...@oss.nttdata.com>>>: >> > > >> > > Hi Chihiro, >> > > >> > > On 2020/02/20 20:20, Chihiro Ito wrote: >> > > > Hi Yasumasa, >> > > > >> > > > Thank you for your quick review. >> > > > >> > > > I modified the code without Properties::store. >> > > > >> > > > Could you review this again, please? >> > > > >> > > > Webrev : >> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.01/ >> > > >> > > - Your change shows "\n" as "\\n". Is it ok? Currently >> "\n" would be shown straightly. >> > > - Your change uses Character::codePointAt to convert >> char to int value. >> > > According to Javadoc, it would be different value if a >> char is in surrogate range. >> > > - Description of serializePropertiesToByteArray() says >> the return value is encoded in ISO 8859-1, >> > > but it does not seems to be so because the logic >> depends on the spec of Properties::store. Is it ok? >> > > - Test case does not stable because system properties >> might be different from your environment. >> > > I suggest you to set system properties for testing >> explicitly. E.g. >> > > -Dnormal=normal_val -D"space space=blank blank" >> -Dnonascii=あいうえお -Dopenjdk_url=http://openjdk.java.net/ -Dbackslash="\\" >> > > * Also I recommend you to check "\n" in the test >> from `line.separator`. I think it is stable property. >> > > >> > > I've not convinced whether we should compliant to the >> comment which says for ISO 8859-1. >> > > If it is important, we can use CharsetEncoder from >> ISO_8859_1 as below: >> > > >> > > >> http://cr.openjdk.java.net/~ysuenaga/JDK-8222489/proposal-encoder/ >> > > >> > > OTOH we can keep current behavior, we can implement more >> simply as below: >> > > (It's similar to yours.) >> > > >> > > >> http://cr.openjdk.java.net/~ysuenaga/JDK-8222489/proposal-props-style/ >> > > >> > > >> > > Thanks, >> > > >> > > Yasumasa >> > > >> > > >> > > > Regards, >> > > > Chihiro >> > > > >> > > > >> > > > 2020年2月20日(木) 9:34 Yasumasa Suenaga < >> suen...@oss.nttdata.com <mailto:suen...@oss.nttdata.com> <mailto: >> suen...@oss.nttdata.com <mailto:suen...@oss.nttdata.com>> <mailto: >> suen...@oss.nttdata.com <mailto:suen...@oss.nttdata.com> <mailto: >> suen...@oss.nttdata.com <mailto:suen...@oss.nttdata.com>>>>: >> > > > >> > > > Hi Chihiro, >> > > > >> > > > I think this problem is caused by spec of >> `Properties::store(Writer)`. >> > > > >> > > > `Properties::store(OutputStream)` says that the >> output format is as same as `store(Writer)` [1]. >> > > > `Properties::store(Writer)` says that `#`, `!`, `=`, >> `:` are written with a preceding backslash [2]. >> > > > >> > > > So I think we should not use `Properties::store` to >> serialize properties. >> > > > >> > > > >> > > > Thanks, >> > > > >> > > > Yasumasa >> > > > >> > > > >> > > > [1] >> https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/util/Properties.html#store(java.io.OutputStream,java.lang.String) >> > > > [2] >> https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/util/Properties.html#store(java.io.Writer,java.lang.String) >> > > > >> > > > >> > > > On 2020/02/19 22:36, Chihiro Ito wrote: >> > > > > Hi, >> > > > > >> > > > > Could you review this tiny fix, please? >> > > > > >> > > > > This problem affected not the only path on >> Windows, but also Linux and URLs using ":". >> > > > > >> > > > > Webrev : >> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.00/ >> > > > > JBS : >> https://bugs.openjdk.java.net/browse/JDK-8222489 >> > > > > >> > > > > Regards, >> > > > > Chihiro >> > > > >> > > >> > >> > >
