Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

> On Sep 26, 2017, at 1:11 AM, Philipp Kunz <philipp.kunz at paratix.ch> 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.

I'm flattered.

> 
> 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.

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. 

> 
> 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.

I don't know if there is a rule. Yours is OK. I said it was just my habit.

> 
> The @modules was a left-over from previous versions.
> 
> I'm not sure where you meant I should put the test to exactly. In http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk 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?

Oh sorry, we've just recently change it to http://hg.openjdk.java.net/jdk10/master/. You don't really need to rework this time, but if you want to continue hacking on OpenJDK, this is the new repo.

> 
> 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.

It is a little uncommon, but that does mean a range.

> 
> 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.

Correct. @test must be there otherwise jtreg will not treat it as a test. @bug might be used by some reporting tools.

> 
> 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.

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

  http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html

> 
> 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.

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

> 
> Regards,
> Philipp