Revised webrev at: http://cr.openjdk.java.net/~jlaskey/8200434/webrev-01/index.html <http://cr.openjdk.java.net/~jlaskey/8200434/webrev-01/index.html>
Thank you Roger, — Jim > On Sep 5, 2018, at 5:07 PM, Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Jim, > > Overall it looks fine. Some quibbles on the wording and the test. > > > The spec for the align() and align(n) methods in String.java show a possibly > misleading inconsistency. > The first line says: > > Removes vertical and horizontal white space margins. > > But later align() says: > > Trailing spaces are preserved. > > The former implies all 4 margins but then it seems only to apply to 3 of the > 4. (top, left, bottom). > I'm not sure if there some wording that can clear that up in the first line. There has been some debate on whether to drop trailing blanks or not. I’m in favour of dropping trailing blanks, but need to see some use cases. > > > AlignIndent.java: > > - Line 28: this test should not need /othervm nor the explicit @compile > > - There should be tests of align(n) with negative values. > > - The for for for for structure doesn't follow the style guide. Fixed, see updated webrev. > > > Thanks, Roger > > (Sorry for the duplicates, I missed core-libs the first time) > > On 8/29/18 10:00 AM, Jim Laskey wrote: >> Please review the code for String::align and String::indent at the link >> below. >> >> Notes: >> Includes a private version of String::isMultiline() which may be made >> into a public method at some future date >> Includes minor correctness clean up of StringLatin1.java, >> StringUTF16.java >> >> webrev: http://cr.openjdk.java.net/~jlaskey/8200434/webrev/index.html >> jbs: https://bugs.openjdk.java.net/browse/JDK-8200434 >> >> Cheers, >> >> — Jim >> >
