Please Review: 6984084 (str) n times repetition of character constructor for java.lang.String

[Alan Bateman]

On 21/08/2012 21:45, Jim Gish wrote:
> Please review 
> http://cr.openjdk.java.net/~jgish/6984084-jdk8-StringRepeat/ 
> <http://cr.openjdk.java.net/%7Ejgish/6984084-jdk8-StringRepeat/>
>
> This started in lambda, making changes to both StringJoiner and 
> String.  However, the dependence of String.repeat() on StringJoiner 
> has been removed and the resulting non-lambda classes moved here for 
> review.  There will be a separate change and review for the 
> StringJoiner changes in lamda-dev.
On the surface then String s = "foo".repeat(5); seems nice. I just 
wonder whether we can get any data on usage of similar methods in 
commonly used libraries. Some languages such as Python have a built-ins 
for this, and I think C# has a ctor, just not clear (to me anyway) how 
often the latter or equivalent it used. Folks here might be able to jump 
in to indicate how often they've needed to do this and whether the 
common-case is a repeated char rather than String (or CharSequence). 
Folks here might also have ideas on getting some usage data in other 
libraries or languages.

I'm less sure about StringBuilder.append(int,CharSequence) as 
sb.append(5,cs) is not much less than "for (int i=0; i<5; i++) 
sb.append(cs);". I also wonder whether it may be more common to want to 
appear a number of chars, sb.append(5, '-') for example.

Anyway, on the javadoc then AbstractStringBuilder.append suggests 
special hanging for n==1 but I think that can be dropped the javadoc.

On the implementation then it would be good to make the style consistent 
with the existing code (4 spaces for indent, etc.). Otherwise it seems okay.

I assume the update to test/Makefile is just for your own local usage 
and that you aren't planning to include this (the jdk_lang target 
already runs the tests in test/java/lang/**).

In Repeat.java (same thing in AppendIntCharSequence.java) then I think 
the -1 test can be simplified to:

try {
     "c".repeat(-1);
     throw new RuntimeException(...);
} catch (IllegalArgumenetException expected) { }

We use GPL on the tests so you need to check AppendIntCharSequence.java.

On test coverage then it looks like StringBuffer is missed. Also the 
assertion that appending null will append "null".

-Alan.