On 07/24/13 08:37, Paul Sandoz wrote:

On Jul 24, 2013, at 12:04 PM, Chris Hegarty <chris.hega...@oracle.com> wrote:

Hi Paul, Doug,

I am happy to be considered as a reviewer for this change.

Thanks!

Thanks to you both!



I think the separation of policies and execution mechanisms from the fluent 
completion-style capabilities is nice.

Some minor comments/questions:
1) Should CS.toCompletableFuture declare UnsupportedOperationException
   in its @throws ?


What is the convention for declaring runtime exceptions? I notice 
Iterator.remove does not declare runtime exceptions.


I think the convention for javadoc is to list if it provides
non-obvious information. Which might marginally hold here, so I added.

2) "All CompletionStage methods are implemented independently of other
    public methods, so the behavior of one method is not impacted by
    overrides of others in subclasses."

    I guess this could be an implNote, but then it might not be as
    conveniently placed.


That policy reads to me like a spec requirement on CF.

Yes, a meta-impl-note of sorts. Even if we were to overhaul the
internals of this class, we'd want to make sure this still
holds. This way people can confidently override for example all non-async
methods, knowing that there is no impact on async, and vice versa.




3) "Actions supplied for dependent completions of non-async methods may
    be performed by the thread that completes the current
    CompletableFuture, or by any other caller of these methods. "

    Is "these methods" referring to other SC methods, or other CF
    methods? It should be the latter, right.

Clarified to:

 * <li>Actions supplied for dependent completions of
 * <em>non-async</em> methods may be performed by the thread that
 * completes the current CompletableFuture, or by any other caller of
 * a completion method.</li>

Thanks also to Chris for pointing out that CompletableFuture
did not have arrangements in place for asyncs when users run with
system property java.util.concurrent.ForkJoinPool.common.parallelism
set to zero. Now it does.

-Doug



Reply via email to