Code review request: JDK-8008670 (partial java.util.stream implementation)

[Peter Levart]

On 03/12/2013 02:29 AM, Mike Duigou wrote:
> Some notes:
>
> - Copyrights update.
>
> - The order of notes on files is the order that I read the files. It seems like not a bad order to review them.
>
> - package docs? Coming next?
>
> java.util.Map::
>
> - @since 1.8 missing for defaults
>
> - <tt></tt> should be replace with {@code}
>
> - default methods need the notice that the default implementations do not provide atomicity. (copy from putIfAbsent)
>
> - ::forEach() - Rename the parameter from block?
>
> - ::computeIfAbsent() - "and enters it into this map" -> inserts?
>
> - I wonder if compute if absent should replace the original get(key) with containsKey(key) and replace get() with containsKey() in the pseudo-code documentation. If implementations know that the map can't contain null then they can optimize to get(). Demonstrated with get() would seem to produce incorrect results with a present null value.
>
> - ::computeIfAbsent()/computeIfPresent()/compute() - "key with which the specified value is to be associated" -> derived value.
>
> - ::computeIfAbsent()/computeIfPresent()/compute() - @throws RuntimeException or Error These should be separate.

Hi,

I would just like to point out to a discussion a couple of months ago:

http://mail.openjdk.java.net/pipermail/lambda-dev/2013-January/007509.html

... at least Map.compute() still has this anomaly when executed against 
HashMap.

Regards, Peter

>
> java.util.Collections::
>
> - Map.synchronizedMap needs extension to provide synchronized implementations of Map default methods.
>
> java.util.Hashtable::
>
> - synchronized implementations of Map default methods are needed.
>
>
> AbstractTask::
>
> - "that describes a portion of the input" -> describes the he portion of the input associated with the subtree rooted at this task.
>
> - setRawResult() : needs better docs. Why does getRawResult return local (which may be null)?
>
> - Are trees of abstract task always homogeneous? Perhaps an assert parent.getClass == class ? since the T can't be verified to be the same as the class.
>
> - onCompletion does not clear numChildren
>
>
>
> ForEachOps::
>
> - BooleanProvider -> BooleanSupplier
>
> - in whatever Thread -> on whatever thread
>
> - "<p/>There is no guarantee that additional elements.." should be before @apiNote.
>
> - Wouldn't it be nice if there as a way to template the primitive impls. :-)
>
>
> IntermediateOp::
>
> - "An intermediate operation has an input type and an output type". These aren't reified or introspectable in contrast to the shape.
>
> - Perhaps defer more about statefulness to StatefulOp
>
>
> StatefulOp::
>
> - Should include a sentence about how a stateful op knows when it has received it's last element.
>
> - Perhaps re-iterate wrapSink() to provide documentation about completion and when elements are written to output. ie. Sink.end()
>
>
> Sink::
>
> - ::end() "If the {@code Sink} buffers any results from previous values, they should dump their contents downstream and clear any stored state." -> If the {@code Sink} is stateful it should send all of it's stored state downstream.
>
> - :: I wonder about the suggestion to clear stored state. Why is this better than just letting it be GCed? If there is value to explicit clearing we should explain why (at least partially).
>
> - ::cancellationRequested() -> :: accepting()? Cancellation sounds like an exceptional state.
>
> - ::cancellationRequested() - document default. @implNote
>
> - why does accept(T) not merit an ISE default?
>
> - ChainedReference/ChainedInt/etc : Should mention in the first sentence that they are abstract. "Abstract skeleton {@code Sink} implementation for creating chains of sinks."
>
>
> Tripwire::
>
> - "Turned off for production." -> "Normally this should be turned off for production systems."
>
>
> Node::
>
> - I don't find the defaults particularly helpful. I would probably prefer them to be fully abstract so that I remember to implement them.
>
> - ::forEach() - worth mentioning that the traversal order is possibly unspecified?
>
> - ::toArray() warning regarding shared array and mutation is not sufficiently dire.
>
> - ::toArray() generator is unexplained. relation between generator needs to be clear. ie. generator might not be called even for unshared.
>
> - ::copyInto() doesn't offer the ability to get some subset of node elements. count() returns long but arrays can only hold INTEGER.MAX_VALUE  (less actually) elements. Either there has to be a way to get partial results or we might as well make count an int.
>
> - ::Builder - is it necessary to say that it is a flat node?
>
> - That's a lotta template code!
>
>
>
> StreamOpFlag::
>
> - tables could be a proper HTML table. If you want me to rework it as a proper 508 compliant table, just ask. :-)
>
>
> On Mar 11 2013, at 14:55 , Brian Goetz wrote:
>
>> I've posted an updated webrev incorporating comments received to date:
>>
>>   http://cr.openjdk.java.net/~briangoetz/jdk-8008670.2/webrev/
>>
>>
>>
>> On 2/21/2013 2:16 PM, Brian Goetz wrote:
>>> At:
>>>    http://cr.openjdk.java.net/~briangoetz/jdk-8008670/webrev/
>>>
>>> there is a webrev of a subset of the implementation of java.util.stream,
>>> for review.  These are all new files.  None of these are public classes;
>>> they are internal interfaces for the Stream library, as well as some
>>> implementation classes.  This is about half the classes in the Streams
>>> package.
>>>
>>> (The webrev includes changes to Map, but those are being reviewed
>>> separately, so just ignore those.)
>>>
>>> We're asking people to focus their review on both the quality of API
>>> specification for these internal APIs, and the quality of implementation
>>> of the specified classes.  It may not be obvious how some of these work
>>> until you've seen the rest of it, but do what you can.
>>>