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