Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Fri, 25 Feb 2022 22:59:05 GMT, liach wrote:
>> src/java.base/share/classes/java/util/Random.java line 298:
>>
>>> 296: */
>>> 297: public static Random from(RandomGenerator random) {
>>> 298: return RandomWrapper.wrap(random);
>>
>> Might be good to check here or in the called methods / constructors for
>> `null`. Currently `null` would not be noticed until the first method is
>> called on the created `Random`, which makes it difficult for the user to
>> track down bugs in their code.
>
> Suggestion:
>
> Objects.requireNonNull(random);
> return RandomWrapper.wrap(random);
>
> fyi this is the original change suggested by marcono1234. Notice that this
> might need a csr update; maybe a javadoc update is needed too
Arguably, `RandomWrapper.wrap` should be inlined here, since it’s not used
anywhere else:
Suggestion:
if (Objects.requireNonNull(random, "random") instanceof Random) {
return (Random) random;
}
return new RandomWrapper(random);
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Sun, 20 Feb 2022 03:15:22 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove missed whitespace
src/java.base/share/classes/java/util/Random.java line 298:
> 296: */
> 297: public static Random from(RandomGenerator random) {
> 298: return RandomWrapper.wrap(random);
Might be good to check here or in the called methods / constructors for `null`.
Currently `null` would not be noticed until the first method is called on the
created `Random`, which makes it difficult for the user to track down bugs in
their code.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Tue, 22 Feb 2022 23:19:14 GMT, Marcono1234 wrote:
>> Yasser Bazzi has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove missed whitespace
>
> src/java.base/share/classes/java/util/Random.java line 298:
>
>> 296: */
>> 297: public static Random from(RandomGenerator random) {
>> 298: return RandomWrapper.wrap(random);
>
> Might be good to check here or in the called methods / constructors for
> `null`. Currently `null` would not be noticed until the first method is
> called on the created `Random`, which makes it difficult for the user to
> track down bugs in their code.
Suggestion:
Objects.requireNonNull(random);
return RandomWrapper.wrap(random);
fyi this is the original change suggested by marcono1234. Notice that this
might need a csr update; maybe a javadoc update is needed too
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Sun, 20 Feb 2022 06:39:26 GMT, liach wrote:
>> Yasser Bazzi has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove missed whitespace
>
> src/java.base/share/classes/java/util/Random.java line 95:
>
>> 93: private static class RandomWrapper extends Random implements
>> RandomGenerator {
>> 94: private final RandomGenerator generator;
>> 95: private final boolean initialized;
>
> Can we create a private or package-private constructor for `Random` for this
> subclass' constructor to call? The no-arg constructor has some unnecessary
> calculations, and the `long` constructor calls `setSeed`. A special `Random`
> constructor for this subclass allows removing this special logic (by not
> calling `setSeed` at all) and avoid redundant seed initialization.
More specifically, in Random class, declare
// special constructor for RandomWrapper to call
private Random(Void unused) {
this.seed = new AtomicLong();
}
So the randomwrapper can be (with `initialized` field removed):
private RandomWrapper(RandomGenerator randomToWrap) {
super(null);
this.generator = randomToWrap;
}
@Override
public void setSeed(long seed) {
throw new UnsupportedOperationException();
}
Currently, the RandomWrapper constructor calls the no-arg super constructor
implicitly, which has some overheads for unnecessary calculations.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Sun, 20 Feb 2022 03:15:22 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > remove missed whitespace At this point I think you should just drop all changes to src/java.base/share/classes/java/util/random/RandomGenerator.java, since they are only cosmetic. Plus you are missing a newline at the end of the file. Otherwise, LGTM. - Changes requested by jlaskey (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Sun, 20 Feb 2022 03:15:22 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove missed whitespace
src/java.base/share/classes/java/util/Random.java line 95:
> 93: private static class RandomWrapper extends Random implements
> RandomGenerator {
> 94: private final RandomGenerator generator;
> 95: private final boolean initialized;
Can we create a private or package-private constructor for `Random` for this
subclass' constructor to call? The no-arg constructor has some unnecessary
calculations, and the `long` constructor calls `setSeed`. A special `Random`
constructor for this subclass allows removing this special logic (by not
calling `setSeed` at all) and avoid redundant seed initialization.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
> Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 Yasser Bazzi has updated the pull request incrementally with one additional commit since the last revision: remove missed whitespace - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/78f55fc5..5dc0e1ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7001/head:pull/7001 PR: https://git.openjdk.java.net/jdk/pull/7001
