On Tue, 17 Nov 2020 21:22:28 GMT, Paul Sandoz <psan...@openjdk.org> wrote:
>> Jim Laskey has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 40 commits: >> >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Update package-info.java >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Updated RandomGeneratorFactory javadoc. >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Updated documentation for RandomGeneratorFactory. >> - Merge branch 'master' into 8248862 >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Move RandomGeneratorProperty >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Clear up javadoc >> - 8248862; Implement Enhanced Pseudo-Random Number Generators >> >> remove RandomGeneratorProperty from API >> - ... and 30 more: >> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68 > > src/java.base/share/classes/java/util/Random.java line 592: > >> 590: >> 591: @Override >> 592: public Spliterator.OfInt makeIntsSpliterator(long index, long >> fence, int origin, int bound) { > > Unsure if this and the other two methods are intended to be public or not, > since they are at the end of the class and override methods of a module > private class. In principle there is nothing wrong with such `Spliterator` > factories, but wonder if they are really needed given the `Stream` returning > methods. The arrangement of classes makes it awkward to hide these methods. Re the properties general comment: I moved properties to RandomSupport based on the notion that the SPI work with come later. Re makeIntsSpliterator: These methods aren't exposed in the java.util.Random API I guess no harm done. The only solution I can think of is to create an intermediate implementor, but that leaves the methods exposed as well. > src/java.base/share/classes/java/util/SplittableRandom.java line 171: > >> 169: * RandomGenerator properties. >> 170: */ >> 171: static Map<RandomGeneratorProperty, Object> getProperties() { > > With records exiting preview in 16 this map of properties could i think be > represented as a record instance, with better type safety, where > `RandomSupport.RandomGeneratorProperty` enum values become typed fields > declared on the record class. Something to consider after integration perhaps? Yes. > src/java.base/share/classes/java/util/SplittableRandom.java line 211: > >> 209: * >> http://zimbry.blogspot.com/2011/09/better-bit-mixing-improving-on.html >> 210: */ >> 211: private static long mix64(long z) { > > Usages be replaced with calls to `RandomSupport.mixStafford13`? We were careful to not change the sequences (from fixed seed) generated by existing prngs. This was an edge case. > src/java.base/share/classes/module-info.java line 250: > >> 248: exports jdk.internal.util.xml.impl to >> 249: jdk.jfr; >> 250: exports jdk.internal.util.random; > > Unqualified export, should this be `to jdk.random`? I guess you are right. Until we have a defined SPI we should restrict. ------------- PR: https://git.openjdk.java.net/jdk/pull/1273
