Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Sat, 29 Jan 2022 16:10:28 GMT, Yasser Bazzi wrote: > Oh sorry about that, my IDE probably did the changes and it went unnoticed by > me, should i revert the offending commit and recommit only the appropriate > changes? Just add a new commit that undoes the unwanted changes. Please don't rebase and force-push (if you were thinking of doing that) because it disrupts the historical comments and incremental diffs. Note that when this is eventually integrated, all the intermediate commits will be "squashed" and a single consolidated commit will be pushed into the JDK mainline. By the way, I'm still thinking about the API factoring issue, and I'm leaning toward having a static factory method on Random instead of a default on RandomGenerator. Still open for discussion though. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Fri, 28 Jan 2022 16:01:56 GMT, Roger Riggs wrote: > (For future reference: As tempting as it is to make other small improvements > to the code such as missing but optional @ Overrides, it obscures the changes > that are the subject of the PR.). Oh sorry about that, my IDE probably did the changes and it went unnoticed by me, should i revert the offending commit and recommit only the appropriate changes? - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Fri, 28 Jan 2022 01:06:47 GMT, liach wrote:
>> Commited the change on df78e05e3e692e2189c9d318fbd4892a4b96a55f
>
> Will we gradually phase out the `Random` class? If yes, we should put this
> conversion method in `Random` so that we don't break the newer API shall
> `Random` be retired.
I don't think Random will ever be removed, so this probably isn't an issue.
There is the overall question of which factoring is preferable: adding a
default method on RandomGenerator, as is done here, or an alternative of adding
a static utility to Random, something like:
// Random.java
public static Random from(RandomGenerator generator) { ... }
This makes the call site a bit longer and adds a level of nesting:
Collections.shuffle(list,
Random.from(RandomGenerator.of("L64X128MixRandom")));
compared to the default method approach:
Collections.shuffle(list,
RandomGenerator.of("L64X128MixRandom").asRandom());
I believe this allows the wrapper class to be placed in java.util, or even as a
nested class of Random, instead of having to be a new top-level class in
jdk.internal.
Another consideration is whether the adapter should be on the "new" thing or on
the "old" thing. There's a little bit of precedent to put adapters on the "old"
thing so that the "new" thing doesn't get polluted with the old. For example,
consider Enumeration::asIterator. (Even though the adaptation is going the
other way in that case, there are still alternative factorings that place the
adapter on the new instead of the old.)
I don't think any of this is compelling but it's worth a discussion.
Regardless of where the adapter ends up, the "other" class should have a note
and a link that refers to the adapter.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Fri, 28 Jan 2022 00:36:01 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 four additional
> commits since the last revision:
>
> - make sure setseed its initialized and throw
> - remove tabs
> - Change name of function from wrapRandom to wrap
> - Change variable name and wording in javadocs
(For future reference: As tempting as it is to make other small improvements to
the code such as missing but optional @ Overrides, it obscures the changes that
are the subject of the PR.).
src/java.base/share/classes/jdk/internal/util/random/RandomWrapper.java line 36:
> 34: /**
> 35: * Class used to wrap a {@link java.util.random.RandomGenerator} to
> 36: * {@link java.util.Random}
Missing period at end of sentence.
src/java.base/share/classes/jdk/internal/util/random/RandomWrapper.java line 42:
> 40: public class RandomWrapper extends Random implements RandomGenerator {
> 41: private final RandomGenerator generator;
> 42: private boolean initialized;
"final" would be appropriate here.
src/java.base/share/classes/jdk/internal/util/random/RandomWrapper.java line 59:
> 57: /**
> 58: * setSeed does not exist in {@link java.util.random.RandomGenerator}
> so can't
> 59: * use it
Missing period.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Fri, 28 Jan 2022 00:36:57 GMT, Yasser Bazzi wrote:
>> Suggest:
>>
>>> Returns an instance of {@link java.util.Random} based on this {@code
>>> RandomGenerator}.
>>> If this generator is already an instance of {@code Random}, it is returned.
>>> Otherwise, this method
>>> returns an instance of {@code Random} that delegates all methods except
>>> `setSeed` to this
>>> generator. Its `setSeed` method always throws {@link
>>> UnsupportedOperationException}.
>>
>> (Note no link is necessary for RandomGenerator since we are in that class
>> already.)
>
> Commited the change on df78e05e3e692e2189c9d318fbd4892a4b96a55f
Will we gradually phase out the `Random` class? If yes, we should put this
conversion method in `Random` so that we don't break the newer API shall
`Random` be retired.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Wed, 26 Jan 2022 17:31:27 GMT, Stuart Marks wrote:
>> Yasser Bazzi has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - make sure setseed its initialized and throw
>> - remove tabs
>> - Change name of function from wrapRandom to wrap
>> - Change variable name and wording in javadocs
>
> src/java.base/share/classes/jdk/internal/util/random/RandomWrapper.java line
> 41:
>
>> 39: @SuppressWarnings("serial")
>> 40: public class RandomWrapper extends Random implements RandomGenerator {
>> 41: private final RandomGenerator randomToWrap;
>
> Suggest renaming the field to "generator" and replacing subsequent calls to
> `this.randomToWrap.foo()` with `generator.foo()`.
Changed to generator on this commit df78e05e3e692e2189c9d318fbd4892a4b96a55f
> src/java.base/share/classes/jdk/internal/util/random/RandomWrapper.java line
> 47:
>
>> 45: }
>> 46:
>> 47: public static Random wrapRandom(RandomGenerator random) {
>
> Probably better if the method is just `wrap` since the class name is
> descriptive enough already.
Commited this change on aadef6f465c0b776f4b8025e2655110b424c6801
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
On Wed, 26 Jan 2022 17:43:09 GMT, Stuart Marks wrote:
>> Is something like this better?
>> /**
>> * Returns a {@link java.util.Random} from this {@link
>> java.util.RandomGenerator}.
>> * The resulting Random acts in all respects except that setSeed is not
>> available.
>> *
>> * @return {@link java.util.Random}
>> */
>
> Suggest:
>
>> Returns an instance of {@link java.util.Random} based on this {@code
>> RandomGenerator}.
>> If this generator is already an instance of {@code Random}, it is returned.
>> Otherwise, this method
>> returns an instance of {@code Random} that delegates all methods except
>> `setSeed` to this
>> generator. Its `setSeed` method always throws {@link
>> UnsupportedOperationException}.
>
> (Note no link is necessary for RandomGenerator since we are in that class
> already.)
Commited the change on df78e05e3e692e2189c9d318fbd4892a4b96a55f
>> throwing UnsupportedOperationException was my first thought but when i call
>> asRandom() it will always throw it, i could create something like what
>> ThreadLocalRandom does (check if it initialized and throw)
>
> Yes, probably following ThreadLocalRandom is the right thing. Pretty clunky
> but I think it's necessary.
ok so i made the change to check if its initialized and throw if the user tries
to use setSeet() c297d42f4505b03780bd9ab2e169546fd6cffcb9
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v2]
> 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 four additional commits since the last revision: - make sure setseed its initialized and throw - remove tabs - Change name of function from wrapRandom to wrap - Change variable name and wording in javadocs - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/fbdf4969..c297d42f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7001&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7001&range=00-01 Stats: 51 lines in 2 files changed: 18 ins; 0 del; 33 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
