-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8369123: Still more small Float16 refactorings #27625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
in the future
Would one remainder suffice for this remainder reminder?
Consider adding a field comment to align with Float, Double and friends.
Other private implementation constants in this class have comments. These would be rather obvious, but perheps we can consider adding them just for consistency? Should we consider ordering private vs. public constants? Now they seem a bit mingled, making it harder for the eye to scan the public API of the class. Line 372:
Are we okay to invoke regex from such low level code? |
Refs: |
Thanks for providing context, it makes the sentence easier to understand. I now parse "remainder operator remainder" as "the remainder function as defined by JVM drem/frem bytecodes". The comment seems a bit terse in its current form, more of a note-to-self reminder of future implementation ideas. Usually comments refer to current functionality, so this was sticking out to me as somewhat unusual. @jddarcy may have an opinion here. |
Upon further inspection, found a few more opportunities to refine the use of constants in the Float16 implementation.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27625/head:pull/27625
$ git checkout pull/27625
Update a local copy of the PR:
$ git checkout pull/27625
$ git pull https://git.openjdk.org/jdk.git pull/27625/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27625
View PR using the GUI difftool:
$ git pr show -t 27625
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27625.diff
Using Webrev
Link to Webrev Comment