Hi Claes,
> so the win comes from forEach on the synchronized collection implicitly
> being synchronized atomically, whereas foreach will synchronize on
> each iterator operation and thus allow racy modification of elements
> in between iteration, right?
Yes, this win will be applied to any type that does something special in
forEach. For instance we have MultiReader collections in Eclipse Collections,
which take a read lock on forEach, and throws UnsupportedOperationException on
iterator. This change would make String.join safe to use for these as well,
without requiring an explicit lock in client code.
>
> Seems reasonable to consider, though it'd have to percolate through to most
> production deployments before you could use it - and even then
> it seems somewhat fragile. And isn't this just one of many utility
> methods that would have to be fixed to avoid issues with hidden
> iterators completely?
>
Yes, this is one small step. Synchronized collections wouldn’t become less
tricky to use, but there would be one less hidden iterator to worry about.
> String.join was recently changed to this by JDK-8265237[1]:
>
> for (CharSequence cs: elements) {
> if (size >= elems.length) {
> elems = Arrays.copyOf(elems, elems.length << 1);
> }
> elems[size++] = String.valueOf(cs);
> }
>
This looks like the code for the overloaded String.join which takes an array. I
was referring to only the version that takes Iterable.
> Of course the body in the new code could be extracted to the same
> effect. The drawback in either implementation is that it would by
> necessity be a capturing lambda, so might add a small allocation
> overhead. I'm biased towards avoiding performance overheads, but maybe
> someone else can make a call on whether this is a worthwhile thing to
> attempt.
>
There is a trade-off here. You remove the need to create an iterator for all
types (due to foreach loop), and replace with forEach with a method reference,
which IIRC might be slightly better than having a lambda. The underlying
collection (let’s say ArrayList) will be called which will also not create an
iterator but iterate directly over the ArrayList internal array.
Thanks,
Don
> Thanks!
>
> /Claes
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8265237
>
> On 2021-08-17 06:55, Donald Raab wrote:
>> The following code:
>> for (CharSequence cs: elements) {
>> joiner.add(cs);
>> }
>> Can be replaced with:
>> elements.forEach(joiner::add);
>> This would have the minor benefit of making it safe to use String.join with
>> synchronized collections without requiring a client side lock. There are
>> likely other opportunities like this in the JDK.
>> Also, with the addition of forEach on Iterable, and Predicate in Java 8, the
>> following design FAQ is outdated.
>> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/doc-files/coll-designfaq.html#a6
>>
>> <https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/doc-files/coll-designfaq.html#a6>
>> Thanks,
>> Don
