8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes #151
Conversation
|
👋 Welcome back jlaskey! A progress list of the required criteria for merging this PR into |
|
@JimLaskey The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
/csr |
|
@JimLaskey has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
| * be greater equal zero and less than {@code bound}. If {@code bound} is a | ||
| * power of two then limiting is a simple masking operation. Otherwise, a | ||
| * new result is re-calculated by averaging the previous result and | ||
| * {@code nextInt()} until the final result is greater equal zero and less |
There was a problem hiding this comment.
I don't see how 'averaging' comes from the invocation of boundedNextInt which appears to choose the first candidate that meets the criteria.
The comment also applies to nextLong overloads.
There was a problem hiding this comment.
If the first value calculated doesn't meet the criteria, then you either mask (power of 2) or go into this loop which does the averaging. The intent is not to bias the outcome.
for (long u = r >>> 1; // ensure nonnegative
u + m - (r = u % n) < 0L; // rejection check
u = rng.nextLong() >>> 1) // retry
;
There was a problem hiding this comment.
Yea you're right. I was focused on the divide by two, which appears to just remove the sign. Clearer interpretation:
r = rng.nextLong();
n = range;
m = n - 1; // mask
for (long u = r / 2; // ensure nonnegative
u + m < r; // rejection check
u = rng.nextLong() / 2) // retry
r = u % n;
|
/help |
|
@JimLaskey Available commands:
|
|
@JimLaskey Command syntax:
Some examples:
If issues are specified only by their ID, the title will be automatically retrieved from JBS. The project prefix ( |
|
@JimLaskey The issue |
|
/issue JDK-8266313 JDK-8268770 |
openjdk
bot
changed the title
|
@JimLaskey This issue is referenced in the PR title - it will now be updated. Adding additional issue to issue list: |
|
/csr |
|
@JimLaskey this pull request will not be integrated until the CSR request JDK-8268770 for issue JDK-8266313 has been approved. |
| * the result to be greater equal {@code origin} and less than {@code bound}. | ||
| * If {@code bound} is a power of two then limiting is a simple masking | ||
| * operation. Otherwise, the result is re-calculated by invoking | ||
| * {@code nextInt()} until the result is greater equal {@code origin} |
There was a problem hiding this comment.
greater equal -> greater than or equal
and in the overloads below (and above)
| * {@link RandomSupport#checkBound checkBound}(bound) and then | ||
| * {@link RandomSupport#boundedNextInt boundedNextInt}(this, bound). | ||
| * @implSpec The default implementation checks that {@code bound} is a | ||
| * positive int. Then invokes {@code nextInt()}, limiting the result to |
There was a problem hiding this comment.
I think if you are refering to the Java type it should be in {@ code xxx }.
"int" - > {@ code int}. Or use "integer".
Ditto "ints", "long", and "longs" in the overridden methods below.
|
/integrate |
|
@JimLaskey This PR has not yet been marked as ready for integration. |
|
/integrate |
|
@JimLaskey This PR has not yet been marked as ready for integration. |
|
/integrate |
|
@JimLaskey This PR has not yet been marked as ready for integration. |
|
/help |
|
@JimLaskey Available commands:
|
|
/help integrate |
|
@JimLaskey Available commands:
|
|
/issue remove JDK-8268770 |
|
@JimLaskey |
|
@JimLaskey This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 122 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit 72db09b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@JimLaskey Pushed as commit 72db09b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The wording of the @implSpec referred to internal methods in the description. The patch rewords the @implSpec to be more descriptive of the algorithm than the methods used.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/151/head:pull/151$ git checkout pull/151Update a local copy of the PR:
$ git checkout pull/151$ git pull https://git.openjdk.java.net/jdk17 pull/151/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 151View PR using the GUI difftool:
$ git pr show -t 151Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/151.diff