Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-18 Thread Stuart Marks
On 6/17/18 1:50 AM, Peter Levart wrote: It's a little strange that the generator function is used to construct an empty array (at least in the default method, but overrides will likely do the same if they want to avoid pre-zeroing overhead) in order to just extract the array's type from it.

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-18 Thread Brian Goetz
+1 from me. > On Jun 14, 2018, at 9:02 PM, Stuart Marks wrote: > > Hi all, > > I wanted to restart review of the changeset for bug JDK-8060192 [1]. The > latest webrev is here: [2]. > > The previous review was from December 2017: [3]. The only difference between > the current changeset and

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-18 Thread Stuart Marks
Tagir Valeev wrote: If you are going to create long-living array which is likely to be empty, it's good to deduplicate them to spare the heap space. This can be easily done via existing toArray method like collection.toArray(MyClass.EMPTY_ARRAY) assuming we have an empty array constant. We

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-18 Thread Stuart Marks
Hi Rémi, On 6/15/18 12:26 AM, Remi Forax wrote: The overrides I had previously provided in specific implementation classes like ArrayList actually are slower, because the allocation of the array is done separately from filling it. This necessitates the extra zero-filling step. Thus, I've

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-17 Thread Peter Levart
On 06/17/18 10:50, Peter Levart wrote: The argument about using (and re-using) a method so that its method reference can be passed to the toArray method without any unchecked casts is equally true for any of the 3 alternatives shown - the method itself might need unchecked casts, but its

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-17 Thread Remi Forax
Hi Tagir, - Mail original - > De: "Tagir Valeev" > À: "Stuart Marks" > Cc: "core-libs-dev" > Envoyé: Samedi 16 Juin 2018 06:01:35 > Objet: Re: RFR(s): 8060192: Add default method Collection.toArray(generator) > Hello! > > If you

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-17 Thread Peter Levart
Hi Stuart, On 06/15/18 03:02, Stuart Marks wrote: Comparing this to the type token alternatives, for simple tasks like converting Collection to String[], things are about equal:     toArray(String[]::new)     toArray(String.class)     toArray(String[].class) However, things are hairier if

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-15 Thread Tagir Valeev
Hello! If you are going to create long-living array which is likely to be empty, it's good to deduplicate them to spare the heap space. This can be easily done via existing toArray method like collection.toArray(MyClass.EMPTY_ARRAY) assuming we have an empty array constant. We use this pattern

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2018-06-15 Thread Remi Forax
- Mail original - > De: "Stuart Marks" > À: "core-libs-dev" > Envoyé: Vendredi 15 Juin 2018 03:02:04 > Objet: RFR(s): 8060192: Add default method Collection.toArray(generator) > Hi all, Hi Stuart, > > I wanted to restart review of the changeset for bug JDK-8060192 [1]. The > latest

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-12 Thread Stuart Marks
On 12/7/17 5:03 PM, Martin Buchholz wrote: (I'm still trying to love this new API) The changes to the jsr166 tck are fine. I'm not convinced that the new implementation for ArrayList is progress.  The current implementation of toArray(T[]) does             // Make a new array of a's

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-12 Thread David Lloyd
On Mon, Dec 11, 2017 at 8:00 PM, John Rose wrote: > I submit to you that such a factory is *not* an IntFunction, because > that only creates half of an array (or 0.01% of one), the empty > version that needs to be populated. A natural array factory API > [...] > The

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-11 Thread John Rose
On Dec 11, 2017, at 6:00 PM, John Rose wrote: > > class Arrays { > // people who want x.toArray can also use x.copy(Arrays::make): > static T[] make(SequenceContent content) { … } Correction; that one needs an array type to know what to make: static T[]

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-11 Thread Stuart Marks
Frankly, I think the JDK is just sloppy here. Most of the existing implementations didn't use @Override -- possibly because they were introduced before annotations existed -- and later on, whoever implemented the overrides of the Java 8 default methods decided to add @Override.

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-11 Thread John Rose
On Dec 7, 2017, at 5:03 PM, Martin Buchholz wrote: > > (I'm still trying to love this new API) > > The changes to the jsr166 tck are fine. > > I'm not convinced that the new implementation for ArrayList is progress. > The current implementation of toArray(T[]) does > >

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-08 Thread Peter Levart
On 12/08/2017 04:09 AM, David Lloyd wrote: Yes! It would be even better for the "toArray(Class)" case if Class had a "getArrayClass()" method which returned the Class, which would allow: return (T[]) Arrays.copyOf(elementData, size, componentType.getArrayClass()); and similar for

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-07 Thread Martin Buchholz
On Thu, Dec 7, 2017 at 4:47 PM, Stuart Marks wrote: > > > On 12/7/17 3:50 PM, Jonathan Bluett-Duncan wrote: > > Looking over http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.2/ >> src/java.base/share/classes/java/util/ArrayList.java.cdiff.html, I'm >> wondering

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-07 Thread David Lloyd
Yes! It would be even better for the "toArray(Class)" case if Class had a "getArrayClass()" method which returned the Class, which would allow: return (T[]) Arrays.copyOf(elementData, size, componentType.getArrayClass()); and similar for other array-backed collections. I never

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-07 Thread Martin Buchholz
(I'm still trying to love this new API) The changes to the jsr166 tck are fine. I'm not convinced that the new implementation for ArrayList is progress. The current implementation of toArray(T[]) does // Make a new array of a's runtime type, but my contents: return (T[])

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-07 Thread Stuart Marks
On 12/7/17 3:50 PM, Jonathan Bluett-Duncan wrote: Looking over http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.2/src/java.base/share/classes/java/util/ArrayList.java.cdiff.html, I'm wondering if the method `ArrayList.toArray(IntFunction)` should have an `@Override` to make it

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-07 Thread Jonathan Bluett-Duncan
Hi Stuart, Looking over http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.2/src/java.base/share/classes/java/util/ArrayList.java.cdiff.html, I'm wondering if the method `ArrayList.toArray(IntFunction)` should have an `@Override` to make it clear that it's overriding the default method in

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-07 Thread Stuart Marks
[Martin: please review changes to the JSR 166 TCK.] Another updated webrev for this changeset: http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.2/ This includes an override of toArray(IntFunction) in the implementation of Arrays.asList(), as requested by Tagir Valeev. Overrides

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-06 Thread Stuart Marks
On 12/5/17 8:41 PM, Bernd Eckenfels wrote: Should the test also check for the case the generator returns under- and oversized arrays? Did you mean that the default implementation (and various overriding implementations) of toArray(generator) should do checks on the array that's returned

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-06 Thread Stuart Marks
On 12/5/17 6:54 PM, Tagir Valeev wrote: I think, CopyOnWriteArrayList and a List returned by Arrays.asList deserve specialized implementation as well. Sure, I can add one to Array.asList right away (as part of this changeset). It's even covered by tests already. I'll work with Martin to

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Bernd Eckenfels
://bernd.eckenfels.net From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of Stuart Marks <stuart.ma...@oracle.com> Sent: Wednesday, December 6, 2017 2:27:44 AM To: core-libs-dev Subject: Re: RFR(s): 8060192: Add default method Collec

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Tagir Valeev
Hello! I think, CopyOnWriteArrayList and a List returned by Arrays.asList deserve specialized implementation as well. With best regards, Tagir Valeev. On Wed, Dec 6, 2017 at 8:27 AM, Stuart Marks wrote: > Revised webrev based on comments from Martin Buchholz and Paul

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Stuart Marks
The signature of the proposed generator is public T[] toArray(IntFunction generator) So I don't think that anything has really changed in this regard; T can still be unrelated to E. Right, the new method doesn't offer any safety compared to toArray(T[]). You can still get

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Stuart Marks
Revised webrev based on comments from Martin Buchholz and Paul Sandoz: http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.1/ I've done a bit of rewriting of the @apiNote sections to clarify the intended use of the various toArray() methods. s'marks

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Paul Sandoz
> On 5 Dec 2017, at 11:19, Stuart Marks wrote: > > > > On 12/5/17 10:47 AM, Paul Sandoz wrote: >> 345 * Suppose {@code x} is a collection known to contain only >> strings. >> 346 * The following code can be used to dump the collection into a >> newly >>

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Stuart Marks
On 12/5/17 10:47 AM, Paul Sandoz wrote: 345 * Suppose {@code x} is a collection known to contain only strings. 346 * The following code can be used to dump the collection into a newly 347 * allocated array of {@code String}: Make it an API note? (same for toArray(T[])

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread Paul Sandoz
> On 4 Dec 2017, at 19:20, Stuart Marks wrote: > > Hi all, > > Please review this small enhancement to add a default method > Collection.toArray(generator) that takes a function that creates the > destination array. This is analogous to Stream.toArray(generator). >

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-05 Thread David Lloyd
On Tue, Dec 5, 2017 at 1:03 AM, Remi Forax wrote: > Dumping an ArrayList into an array of String is fairly frequent, i > think. > > The main issue with the current API, T[] toArray(T[] array), is that T > can be unrelated to E (the type of the element in the collection) so

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-04 Thread Stuart Marks
+// no spec changes relative to supertype +public T[] toArray(IntFunction generator) { You probably at least need @inheritDoc for the unchecked exception throws (as I've forgotten many times) There's a new javadoc option "--override-methods summary" that was recently added

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-04 Thread Remi Forax
Hi Martin, - Mail original - > De: "Martin Buchholz" <marti...@google.com> > À: "Stuart Marks" <stuart.ma...@oracle.com> > Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> > Envoyé: Mardi 5 Décembre 2017 05:26:02 > Ob

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-04 Thread Martin Buchholz
On Mon, Dec 4, 2017 at 8:26 PM, Martin Buchholz wrote: > > The biggest question is whether Collection.toArray(generator) pulls its > weight, especially in view of https://shipilev.net/blog/ > 2016/arrays-wisdom-ancients. > Oh wait - Aleksey actually links to this bug!

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-04 Thread Martin Buchholz
The needToWorkAround6260652 changes ought to be in a separate changeset. The biggest question is whether Collection.toArray(generator) pulls its weight, especially in view of https://shipilev.net/blog/2016/arrays-wisdom-ancients. I rarely want to dump elements into a typed array. Dumping into

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

2017-12-04 Thread Martin Buchholz
+// no spec changes relative to supertype +public T[] toArray(IntFunction generator) { You probably at least need @inheritDoc for the unchecked exception throws (as I've forgotten many times) On Mon, Dec 4, 2017 at 7:20 PM, Stuart Marks wrote: > Hi all, >