Hi Yasumasa, I appreciate that you have reviewed so many times.
Regards, Chihiro 2020年2月23日(日) 11:10 Yasumasa Suenaga <suen...@oss.nttdata.com>: > Hi Chihiro, > > Looks good. > Thank you for your updates and patience! > > > Yasumasa > > > On 2020/02/23 0: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 <mailto: > 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> <mailto: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>> <mailto: > 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>>> <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 <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 > > > > > > > > > > > > > > >
