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?
Regards,
Chihiro
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
> > >
> >
>
|
