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
