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.
