Hi, Martin. > 6633613: (str) StringCoding optimizations to avoid unnecessary array > copies with Charset arg
Not necessarily your problem, but it would be nice if your "Subject:" line contained the word "review" or "review request" somewhere in it. It would make this message easier to find. I suppose I should come up with a recommendation, float it about, and get it written up in the appropriate section of the Guide. > with the patches below (a more ambitious patch will hopefully follow > later): Presumably I'll see that review later under one of the charset bugids? > First, warning suppression: I don't believe that StringCoding.java produces build warnings using the default javac options. Should I assume that you're talking about eliminating all build warnings produced using "-Xlint:all" for this file only (not all of the String* files)? > diff --git a/src/share/classes/java/lang/StringCoding.java > b/src/share/classes/java/lang/StringCoding.java These changes are fine. > second, actual fix: > > diff --git a/src/share/classes/java/lang/StringCoding.java > b/src/share/classes/java/lang/StringCoding.java The code you modified was added _extremely_ late during jdk6 development to fix another bug. There's a regression test for that bug. Hopefully you ran it? (Not that I expect it to fail... ) Speaking of regression tests, you didn't include one in these diffs and I don't see an appropriate "noreg-*" keyword in the bug. Please resolve this. See Step 6 ofthe Guide, "Change Planning and Guidelines" [1] for a list of possible keywords. The supplied fix is the minimal amount of change necessary to address this bug (as indicated in your evaluation). Assuming that you still intend to further modify code to improve performance in this area, please remember to add the relevant bugids to the evaluation (or at the very least, references to these bugs in the "See Also" section). Finally, I know that we don't have an externally-visible repository for webrevs yet, but we do need to keep a webrev of these changes as it would aid review should we need to backport this fix. Thanks, iris [1] http://openjdk.java.net/guide/changePlanning.html
