RFR - JDK-8200434 - String::align, String::indent (code review)

Roger Riggs roger.riggs at oracle.com
Wed Sep 5 20:07:52 UTC 2018


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.


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.


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
>



More information about the core-libs-dev mailing list