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 <[email protected]
<mailto:[email protected]>>:
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 <[email protected]
<mailto:[email protected]> <mailto:[email protected]
<mailto:[email protected]>>>:
>
> 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
>
