On Wed, 21 Apr 2021 13:13:16 GMT, Jim Laskey <jlas...@openjdk.org> wrote:

>> Move makeXXXSpilterator from public (@hidden) to protected. No API ch
>
> Jim Laskey has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains eight additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into 8265137
>  - Remove @hidden
>  - Correct the hierarchy of Random
>  - Remove extraneous references to makeXXXSpliterator
>  - Move makeXXXSpliterator methods to RandomSupport
>  - change static final from 'proxy' to 'PROXY'
>  - Make makeXXXSpliterator final
>  - Move makeXXXSpilterator from public (@hidden) to protected. No API change.

Overall looks good.

AbstractSpliteratorGenerator now is a bit weird because it's a utility class 
that has static methods, _and_ it's an abstract superclass that has instance 
methods that mostly just call the static methods. Seems to me that additional 
cleanup is possible. It might not be worth it right now, since the main point 
of this PR -- to remove "leakage" of these helper methods into the public API 
-- has been achieved.

The RandomXXXSpliterator nested classes could also stand some scrutiny. The 
constructors consume a RandomGenerator, which is stored in an instance 
variable. Various comments still refer to this as an 
AbstractSpliteratorGenerator. (See line 961 for example; can't comment directly 
because it's not part of this commit.)

But it's not clear to me that the RandomXXXSpliterator classes really need a 
full RandomGenerator. For example, as far as I can see, RandomIntSpliterator 
_only_ needs `nextInt` to generate int values; therefore it could be 
`IntSupplier`. Similar for long and double. (I'm applying the Interface 
Segregation Principle here.) But again this is probably a cleanup for another 
day.

src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 
1433:

> 1431:         private AbstractSpliteratorGenerator() {
> 1432:         }
> 1433: 

This comment is wrong now since there are instances of this class (really, 
subclasses). The constructor should probably simply be removed.

-------------

Marked as reviewed by smarks (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3469

Reply via email to