On Mon, 13 May 2024 13:06:04 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 190:
>>
>>> 188: if (properties == null) { // double-checking idiom
>>> 189: RandomGeneratorProperties props =
>>> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class);
>>> 190: Objects.requireNonNull(props, rgClass + " missing
>>> annotation");
>>
>> Hello Raffaello, with the `RandomGenerator` implementations now all residing
>> within the java.base module, I think an additional advantage of that is
>> that, it will now allow us to remove this internal
>> `RandomGeneratorProperties` annotation and thus this reflection code.
>>
>> I think one way to do that would be something like this within this
>> `RandomGeneratorFactory` class itself:
>>
>>
>> private record RandomGenEntry(Class<?> randomGenClass, int i, int j,
>> int k, int equiDistribution, boolean
>> stochastic,
>> boolean hardware) {
>>
>> }
>>
>> private static final Map<String, RandomGenEntry> FACTORY_MAP = ... //
>> construct the map
>>
>>
>> where the `FACTORY_MAP` will be keyed to the alogrithm and the value will be
>> a record which holds these additional details about the `RandomGenerator`.
>> This current PR is about getting rid of ServiceLoader usage. So if you want
>> to remove the usage of this annotation and reflection is a separate PR
>> that's fine with me. Furthermore, although I don't see the necessity of an
>> annotation for what we are doing here, if you think that the removal of the
>> annotation and reflection isn't worth doing, that is OK too.
>
> Thinking a bit more, I think we can even get rid of the reflection used in
> `create()` methods implementation, if we wanted to, by doing something like
> this:
>
>
> private record RandomGenEntry(Class<? extends RandomGenerator>
> randomGenClass, int i, int j,
> int k, int equiDistribution, boolean
> stochastic,
> boolean hardware) {
>
> RandomGenerator create() {
> String algo = randomGenClass.getSimpleName();
> return switch (algo) {
> case "Random" -> new Random();
> case "L128X1024MixRandom" -> new L128X1024MixRandom();
> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
> // ... so on for the rest
> default -> throw new InternalError("should not happen");
> };
> }
>
> RandomGenerator create(long seed) {
> String algo = randomGenClass.getSimpleName();
> return switch (algo) {
> case "Random", "SplittableRandom", "SecureRandom" -> {
> throw new UnsupportedOperationException("cannot construct
> with a long seed");
> }
> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
> // ... so on for the rest
> default -> throw new InternalError("should not happen");
> };
> }
>
> RandomGenerator create(byte[] seed) {
> String algo = randomGenClass.getSimpleName();
> return switch (algo) {
> case "Random", "SplittableRandom", "SecureRandom" -> {
> throw new UnsupportedOperationException("cannot construct
> with a byte[] seed");
> }
> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
> // ... so on for the rest
> default -> throw new InternalError("should not happen");
> };
> }
> }
>
>
> private static final Map<String, RandomGenEntry> FACTORY_MAP = ... //
> construct the map
@jaikiran I agree that we can simplify even more. I just wanted to change as
little as possible in this PR to facilitate reviews.
Shall I push your proposed changes in this PR or is a followup PR preferable?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598459168
