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