RFR JDK-8010096 : Initial java.util.Spliterator putback

[Paul Sandoz]

Hi Alan,

Thanks for taking a look.

On Apr 1, 2013, at 6:13 PM, Alan Bateman <Alan.Bateman at oracle.com> wrote:

> On 28/03/2013 15:59, Paul Sandoz wrote:
>> Hi,
>> 
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8010096
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~psandoz/lambda/spliterator/jdk-8010096/webrev/
>> 
>> Spec diff:
>> http://cr.openjdk.java.net/~psandoz/lambda/spliterator/jdk-8010096/specdiff/overview-summary.html
>> 
>> Relevant JavaDoc generated from lambda repo (required for viewing @apiNote, @implSpec, @implNote declarations):
>> http://cr.openjdk.java.net/~psandoz/lambda/spliterator/jdk-8010096/api/java/
>> 
>> Note: some of the JavaDoc generated from the lambda repo may contain additional methods or specification that is relevant to the stream framework.
>> 
> Just a few small comments from a quick pass over the webrev.
> 
> The @return for the Array.spliterator methods reads "A spliterator from an array", maybe this should be "the array".
> 

OK.


> For Iterable.forEach (and the Spliterator methods that take a Consumer) then you might consider "Errors or runtime exceptions" rather than "Exceptions" so that it is clear that an Error thrown by the action's accept method will be propagated too.
> 

I thought that might be redundant but there is sometimes confusion around what things can be thrown from methods that don't declare they throw anything so it is probably worth being explicit.


> The class description for the PrimitiveIterator.OfXXX classes is the one-liner "Specialization for XXX elements". It might be nicer to have this as "A Spliterator specialized for XXX elements".
> 
> Will you change the Tripwire utility class to use the platform logger before you push this?
> 

Yes.


> Another thing on Tripwire is that the property lookup will probably fail if the class is initalized with application code on the stack and there is a security manager set. I think this will need to be changed to get the property value in a privileged blocked.
> 

Ah!


> The copyright header on the tests is normally the GPL header (no "Classpath" exception). I assume you'll fix this before pushing.
> 

OK, grr i always forget that (makes it harder to have an automated template in the IDE when creating a new class).


> One question on the spliteratorDataProvider - what is the issue with IdentityHashMap or do we know yet?
> 

I do not. I will need to update the equivalent test-ng test since the IdentityHashMap occasionally fails. (Keeping duplicates at the moment because it is very convenient to run the lambda test-ng tests.)

Paul.