<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Max<br>
<br>
This time I got it with readAllBytes. Thank you for the hint.<br>
<br>
Apparently, UTF characters are allowed in source code, particularly
in identifiers here, which also has caused the bug. Even if only for
sending patches around I changed it and was surprised to see
escaping working not only in strings but also in identifiers.<br>
<br>
When I had another look at the test I came to the conclusion that it
does not need what has been named refClassFileName before. The
purpose of the test is only to check a signature of a class with a
two byte character in its name and not at the same time to verify
that if that test failed it is specifically because of the name. If
it fails there is a problem no matter why. In the beginning it was
handy to see the difference but I don't think it should be kept and
maintained so I removed it. For the update signature case a second
file to sign is still required though.<br>
<br>
I considered multi-byte a one word before but now I also prefer it
with a capital b. Anyway, this name might not be the best choice and
I changed it to LineBrokenMultiByteCharacter.<br>
<br>
See attached patch.<br>
<br>
Regards,<br>
Philipp<br>
<br>
<p><br>
</p>
<div class="moz-cite-prefix">On 26.09.2017 06:19, Weijun Wang wrote:<br>
</div>
<blockquote
cite="mid:EEECE6A7-BCE2-49CB-B145-A6AB557A0236@oracle.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">On Sep 26, 2017, at 1:11 AM, Philipp Kunz <a class="moz-txt-link-rfc2396E" href="mailto:philipp.kunz@paratix.ch"><philipp.kunz@paratix.ch></a> wrote:
Hi Max
Thank you for the detailed assistance and I really hope it doesn't annoy you too much with so many beginner's mistakes. Every little point of yours seems to be absolutely justified in my point of view.
</pre>
</blockquote>
<pre wrap="">
I'm flattered.
</pre>
<blockquote type="cite">
<pre wrap="">
I did not understand where I could apply the readAllBytes. In case it still applies, would you give me another hint? I'd rather expect some potential for butifying verifyClassNameLineBroken but then I haven't found any really.
</pre>
</blockquote>
<pre wrap="">
I meant you can change "while (inputStream.read() > -1);" to "inputStream.readAllBytes()". Yours is fine. Some people do not like the return value wasted and the auto-increment mechanism in the method is unnecessary here.
</pre>
<blockquote type="cite">
<pre wrap="">
About where to place the declaration of nameBuf I was a little confused probably/obviously when there was a ByteArrayOutputStream before which, however, actually was not used into assuming that reusing the same buffer for all sections would result in better performance or so but that's certainly not a valid assumption now as I consider it again.
About the import static you convinced me but for statically importing java.nio.charset.StandardCharsets.UTF_8 which I prefer this way still. After you wrote I should decide myself, I removed other static imports. If there was kind of an unwritten rule not to use static imports generally that would be a compelling reason but I didn't bother to search for import static through the jdk sources.
</pre>
</blockquote>
<pre wrap="">
I don't know if there is a rule. Yours is OK. I said it was just my habit.
</pre>
<blockquote type="cite">
<pre wrap="">
The @modules was a left-over from previous versions.
I'm not sure where you meant I should put the test to exactly. In <a class="moz-txt-link-freetext" href="http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk">http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk</a> I cant find the parent folder sun you mentioned. In case there is some reorganization in progress not having arrived yet at the tip, could the new test be moved together with it? Or did you mean I should create the sun folder but then the existing tests would not be next to the new one? Did I look in the wrong place?
</pre>
</blockquote>
<pre wrap="">
Oh sorry, we've just recently change it to <a class="moz-txt-link-freetext" href="http://hg.openjdk.java.net/jdk10/master/">http://hg.openjdk.java.net/jdk10/master/</a>. You don't really need to rework this time, but if you want to continue hacking on OpenJDK, this is the new repo.
</pre>
<blockquote type="cite">
<pre wrap="">
When updating the copyright years I was not sure if I should let the existing one, 2011, there in ManifestDigester or replace it with the current year which I did. When looking into other class files I saw none with more than two years but "," looks a little odd to me for indicating a range.
</pre>
</blockquote>
<pre wrap="">
It is a little uncommon, but that does mean a range.
</pre>
<blockquote type="cite">
<pre wrap="">
I assumed that I remove the JDK- prefix from the @bug tag and not the @test tag unless @bug is considered part of the @test.
</pre>
</blockquote>
<pre wrap="">
Correct. @test must be there otherwise jtreg will not treat it as a test. @bug might be used by some reporting tools.
</pre>
<blockquote type="cite">
<pre wrap="">
Is it correct, that a maximum line width of 80 characters is the convention? In case I added some more breaks for that. I just wonder what other style guides I should have respected.
</pre>
</blockquote>
<pre wrap="">
Yes that is the convention. Sometimes a line can be longer if wrapping it is very ugly. In general, don't make a Sdiffs page in a webrev non-readable. A Sdiffs page example is here
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html">http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html</a>
</pre>
<blockquote type="cite">
<pre wrap="">
Just in case you think it would be more efficient I'd not object that a reviewer or some else would just change these little things to get over with it. On the other hand this way I learn it. If there is a suitable documentation that much detailed, I'd be glad if you could point me at it. That might save a few cycles.
</pre>
</blockquote>
<pre wrap="">
I don't know if there is a more suitable doc.
I have some small suggestions:
1. In fact I don't know exactly if a non-ASCII is allowed in source code now. For safety, please change the é char to \u1234 style.
2. Maybe it's better to capitalize "b" in the test name?
3. The @see tag has its format, you can just use "See...". The method name in @link has a wrong signature.
* @see also eAcute in
* {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}
4. Several "throws Exception" should be indented by 8 chars.
5. There are several trailing spaces in your patch. Maybe because of copy-and-paste.
Please send me the new patch as an attachment. That will be more precise and more formal. Please send me what the exact "name <email>" you want to see in the Contributed-by field of the changeset.
Thanks
Max
</pre>
<blockquote type="cite">
<pre wrap="">
Regards,
Philipp
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<title></title>
<br>
<br>
Gruss Philipp<br>
<br>
<br>
<br>
<hr size="2" width="100%"><br>
<img shrinktofit="true"
src="cid:part1.AC31127C.90A9E7A2@paratix.ch" align="bottom"
border="0"><br>
<br>
Paratix GmbH<br>
St Peterhofstatt 11<br>
8001 Zürich<br>
<br>
+41 (0)76 397 79 35<br>
<a href="mailto:philipp.kunz@paratix.ch">philipp.kunz@paratix.ch</a>
</div>
</body>
</html>