8357063: Document preconditions for DecimalDigits methods #25246

wenshao · 2025-05-15T06:24:44Z

Similar to PR #24982
Document preconditions on certain DecimalDigits methods that use operations either unsafe and/or without range checks.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8357063: Document preconditions for DecimalDigits methods (Enhancement - P4)

Reviewers

Volkan Yazici (@vy - Author)
Chen Liang (@liach - Reviewer)
Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25246/head:pull/25246
$ git checkout pull/25246

Update a local copy of the PR:
$ git checkout pull/25246
$ git pull https://git.openjdk.org/jdk.git pull/25246/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25246

View PR using the GUI difftool:
$ git pr show -t 25246

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25246.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-15T06:25:44Z

👋 Welcome back swen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-05-15T06:26:23Z

@wenshao 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:

8357063: Document preconditions for DecimalDigits methods

Reviewed-by: vyazici, liach, rriggs

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 56 new commits pushed to the master branch:

bd99525: 8356193: Remove tests from ProblemList-enable-preview.txt fixed by JDK-8344706
6818dcc: 8351230: Collections.synchronizedList returns a list that is not thread-safe
6a58af3: 8357143: New test AOTCodeCompressedOopsTest.java fails on platforms without AOT Code Cache support
... and 53 more: https://git.openjdk.org/jdk/compare/5e50a584744d316dd881c9404f75e65f31bb0e75...master

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 master branch, type /integrate in a new comment.

openjdk · 2025-05-15T06:26:55Z

@wenshao The following label will be automatically applied to this pull request:

core-libs

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.

mlbridge · 2025-05-15T16:51:17Z

Webrevs

rgiulietti · 2025-05-15T18:12:28Z

@wenshao I understand this is a kind of followup of #24982, but why this title? It seems just renaming some methods with more speaking names.

wenshao · 2025-05-15T21:59:34Z

@wenshao I understand this is a kind of followup of #24982, but why this title? It seems just renaming some methods with more speaking names.

Yes, it is just a renaming of some methods, the title is followup #24982, does the same thing, so uses the same title

rgiulietti · 2025-05-16T07:44:05Z

The renaming is nice and useful.
But #24982, in addition to renaming, emphasizes the expectations about correct usage with bold WARNINGs in the JavaDoc. That's the "document preconditions" in the title.
Adding similar warnings in the JavaDoc, rather than in plain comments in the methods, would be clearer.

vy · 2025-05-16T07:57:57Z

src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

     */
-    public static int getCharsUTF16(long i, int index, byte[] buf) {
+    public static int uncheckedGetCharsUTF16(long i, int index, byte[] buf) {
        // Used by trusted callers.  Assumes all necessary bounds checks have been done by the caller.


Regarding all methods that are prefixed with unchecked in this PR, IMHO:

Precondition warnings should better be moved to the JavaDoc (ala 8353197: Document preconditions for JavaLangAccess methods #24982)

We shall also consider implementing these preconditions using assert statements

vy

@wenshao, thanks for the kind effort – dropped suggestions to match the messages with the ones in JLA.

I prefer all¹ public, unchecked-prefixed DecimalDigits methods to start with a sufficient assert preamble, to fail as early as possible. Though I don't want to create redundant extra work, hence double checking: @jaikiran, @minborg, @liach, WDYT?

¹ Currently, only two private methods are introduced asserts, which are fine.

src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

liach · 2025-05-16T14:10:57Z

Re assert preamble for public methods: Unfortunately this is not possible. The algorithm requires the chars to be written on demand, and it is very costly to precompute the length, aside from the fact that you can easily mess up on precomputation. The uncheckedness is from the 2 private methods, so I think safeguarding the 2 private methods with assertions is sufficient.

Co-authored-by: Volkan Yazıcı <[email protected]>

liach

This version looks good to me. Please wait for a second confirmation. (Allowing authors like @vy to review too)

/reviewers 2

openjdk · 2025-05-16T16:43:40Z

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

RogerRiggs

Useful renames and comment placement improvements. lgtm.

jaikiran · 2025-05-20T15:53:34Z

src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

     * @return index of the most significant digit or minus sign, if present
     */
    public static int getChars(long i, int index, char[] buf) {
-        // Used by trusted callers.  Assumes all necessary bounds checks have been done by the caller.


Hello Shaojin, I think this was a misplaced comment previously. Looking at the implementation of this method, there's no "unsafe" access happening in this method's implementation. It ends up calling putChar which does a Java style array access and thus is backed by the language's bounds checking.

Removing this comment I believe is the right thing. Having said that, I am unsure the javadoc comment of this method should refer to DecimalDigits#uncheckedGetCharsUTF16 because that is confusing and misleading.

Should we change the javadoc text of this method to:

Places characters representing the long i into the character array buf. The characters are placed into the buffer backwards starting with the least significant digit at the specified index (exclusive), and working backwards from there.

Would that accurately describe what this method's implementation currently does?

This method has the same algorithm as uncheckedGetCharsUTF16, the only difference is the safe array access of char[] and the unsafe access of byte[] by uncheckedPutPairUTF16.

This method was also copied from uncheckedGetCharsUTF16 and then modified when the code was written, so I think the reference here is OK.

wenshao · 2025-05-21T22:38:03Z

/integrate

openjdk · 2025-05-21T22:39:13Z

Going to push as commit 07871cd.
Since your change was applied there have been 103 commits pushed to the master branch:

f687cac: 8353504: CDS archives are not found when JVM is in non-variant location
3ee1447: 8357503: gcbasher fails with java.lang.IllegalArgumentException: Unknown constant pool type
4d70689: 8351993: VectorShuffle access to and from MemorySegments
... and 100 more: https://git.openjdk.org/jdk/compare/5e50a584744d316dd881c9404f75e65f31bb0e75...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-21T22:39:20Z

@wenshao Pushed as commit 07871cd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the core-libs [email protected] label May 15, 2025

wenshao force-pushed the dec_unchecked_202505 branch from 792d16f to d40cad7 Compare May 15, 2025 07:39

Document preconditions for DecimalDigits methods

66f71c5

wenshao force-pushed the dec_unchecked_202505 branch from d40cad7 to 66f71c5 Compare May 15, 2025 07:41

wenshao marked this pull request as ready for review May 15, 2025 16:41

wenshao changed the title ~~Document preconditions for DecimalDigits methods~~ 8357063: Document preconditions for DecimalDigits methods May 15, 2025

openjdk bot added the rfr Pull request is ready for review label May 15, 2025

vy reviewed May 16, 2025

View reviewed changes

wenshao added 2 commits May 16, 2025 17:31

document add warnings

381979d

assert

ddb44aa

vy reviewed May 16, 2025

View reviewed changes

wenshao and others added 8 commits May 17, 2025 00:00

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

f88d235

Co-authored-by: Volkan Yazıcı <[email protected]>

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

121695a

Co-authored-by: Volkan Yazıcı <[email protected]>

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

a31d965

Co-authored-by: Volkan Yazıcı <[email protected]>

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

c281623

Co-authored-by: Volkan Yazıcı <[email protected]>

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

27e883c

Co-authored-by: Volkan Yazıcı <[email protected]>

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

aa9d7a3

Co-authored-by: Volkan Yazıcı <[email protected]>

Update src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

bcb9af2

Co-authored-by: Volkan Yazıcı <[email protected]>

warning

da865e4

liach approved these changes May 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 16, 2025

openjdk bot removed the ready Pull request is ready to be integrated label May 16, 2025

vy approved these changes May 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 19, 2025

RogerRiggs approved these changes May 19, 2025

View reviewed changes

jaikiran reviewed May 20, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 21, 2025

openjdk bot closed this May 21, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 21, 2025

8357063: Document preconditions for DecimalDigits methods #25246

8357063: Document preconditions for DecimalDigits methods #25246

Uh oh!

Conversation

wenshao commented May 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 15, 2025

Uh oh!

openjdk bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 15, 2025

Uh oh!

mlbridge bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

rgiulietti commented May 15, 2025

Uh oh!

wenshao commented May 15, 2025

Uh oh!

rgiulietti commented May 16, 2025

Uh oh!

vy May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liach commented May 16, 2025

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 16, 2025

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran May 20, 2025

Choose a reason for hiding this comment

Uh oh!

wenshao May 20, 2025

Choose a reason for hiding this comment

Uh oh!

wenshao May 21, 2025

Choose a reason for hiding this comment

Uh oh!

wenshao commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

Uh oh!

wenshao commented May 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 15, 2025 •

edited

Loading

mlbridge bot commented May 15, 2025 •

edited

Loading

vy May 16, 2025 •

edited

Loading