RFR: String.join(), StringJoiner additions

[Alan Bateman]

On 11/04/2013 23:33, Jim Gish wrote:
> Please review 
> http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/ 
> <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7175206-7172553/>
>
> These are changes that we made in lambda that we're now bringing into 
> JDK8.
>
> I've made a couple of additions - making StringJoiner final and adding 
> a couple of constructors to set the emptyOutput chars.
>
> Thanks,
>    Jim
>
One question on the bug numbers. Are you proposing to use all three? I 
assume that at least 7175206 is not needed for the push to jdk8.

Just a few comments on String.join (which overall, it nice and simple).

The javadoc doesn't make it obvious that the elements can include null, 
are you planning to make that clear?

A minor nit in the first example there is there is a space before the 
closing parentheses.

It looks to be a bit odd to have one of the @see tags before the @throws 
and the other after it.

In one method the @return wraps around without indenting, in the other 
method the first @param is nicely indented but the second one is pushed 
in by extra space.

The examples in the second String.join can probably use diamond. I don't 
know how this looks in the generated javadoc but might be clearer to put 
each of the strings.add on their own lines. There is also 
inconsistencies with spaces and parentheses in these examples.

I see you proposing to put the test for String.join in 
test/java/lang/StringTest.java but that is inconsistent with the current 
organization. I realize this is TestNG test that is not in the unnamed 
package but test/java/lang is just too high up in the tree for a 
specific test. Also "StringTest" is a bit general a name given that it 
is specific to String.join.

The usual copyright header for tests is the pure GPL header.

I plan to look at StringJoiner in more detail later.

-Alan