8286378: Address possibly lossy conversions in java.base #8642

RogerRiggs · 2022-05-10T21:32:10Z

PR#8599 8244681: proposes to add compiler warnings for possible lossy conversions
From the CSR:

"If the type of the right-hand operand of a compound assignment is not assignment compatible with the type of the variable, a cast is implied and possible lossy conversion may silently occur. While similar situations are resolved as compilation errors for primitive assignments, there are no similar rules defined for compound assignments."

This PR anticipates the warnings and adds explicit casts to replace the implicit casts.
In most cases, the cast matches the type of the right-hand side to type of the compound assignment.
Since these casts are truncating the value, there might be a different refactoring that avoids the cast
but a direct replacement was chosen here.

(Advise and suggestions will inform similar updates to other OpenJDK modules).

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

JDK-8286378: Address possibly lossy conversions in java.base

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to e72ce85c
Xue-Lei Andrew Fan (@XueleiFan - Reviewer) ⚠️ Review applies to e72ce85c
Brian Burkhalter (@bplb - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642
$ git checkout pull/8642

Update a local copy of the PR:
$ git checkout pull/8642
$ git pull https://git.openjdk.java.net/jdk pull/8642/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8642

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8642.diff

Add explicit casts to replace implicit casts

bridgekeeper · 2022-05-10T21:32:24Z

👋 Welcome back rriggs! 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 · 2022-05-10T21:34:34Z

@RogerRiggs The following labels will be automatically applied to this pull request:

core-libs
i18n
net
nio
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-05-10T21:38:18Z

Webrevs

naotoj

Looks good. Assuming copyright years will be updated.

src/java.base/share/classes/java/time/es

openjdk · 2022-05-10T22:00:32Z

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

8286378: Address possibly lossy conversions in java.base

Reviewed-by: naoto, xuelei, bpb, alanb

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

46a775a: 8286540: Build failure caused by missing DefaultPollerProvider implementation on AIX
4ad8cfa: 8286002: Add support for intel syntax to capstone hsdis
cec23b1: 8286556: Remove EagerInitialization develop option
89de756: 8286387: Remove unused FreeListAllocator::reduce_free_list
ae695d6: 8286476: x86_32: Fix crashes with non-preview mode after JDK-8284161 (Virtual Threads)
87f3d2b: 8286446: PPC64: fix crashes after JDK-8284161 (virtual threads preview)
7a2bbbb: 8286396: Address possibly lossy conversions in jdk.management.jfr
9ac52b0: 8286392: Address possibly lossy conversions in jdk.jfr
f628966: 8286541: JFR: RecordingFile.write is missing "since 19"
f1554fc: 8285872: JFR: Remove finalize() methods
... and 9 more: https://git.openjdk.java.net/jdk/compare/070a0cda31f83a3cf18a612b91c30d66cf407c3c...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.

XueleiFan

The update in security area looks good to me.

bplb

IO, math, and NIO changes look fine.

src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java

src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java

src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java

asotona · 2022-05-11T08:03:18Z

I can confirm this patch clears all warnings from java.base.

Fixed cast style to add a space after cast, (where consistent with file style) Improved code per review comments in PollSelectors.

RogerRiggs · 2022-05-12T16:49:30Z

/integrate

openjdk · 2022-05-12T16:50:48Z

Going to push as commit 17c5278.
Since your change was applied there have been 44 commits pushed to the master branch:

0a6832b: 8286617: Improve parameter names in javax.lang.model utility visitors
2c5d136: 8282191: Implementation of Foreign Function & Memory API (Preview)
3be394e: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
cc7560e: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen"
82aa045: 8286015: JFR: Remove jfr.save.generated.asm
1904e9d: 8286423: Destroy password protection in the example code in KeyStore
e4439ca: 8284283: javac crashes when several transitive supertypes are missing
752ad1c: 8286422: Add OIDs for RC2 and Blowfish
36bdd25: 8286573: Remove the unnecessary method Attr#attribTopLevel and its usage
dea6e88: 8284680: sun.font.FontConfigManager.getFontConfig() leaks charset
... and 34 more: https://git.openjdk.java.net/jdk/compare/070a0cda31f83a3cf18a612b91c30d66cf407c3c...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-12T16:51:05Z

@RogerRiggs Pushed as commit 17c5278.

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

ExE-Boss · 2022-05-13T04:41:03Z

src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java

                    // timed poll interrupted so need to adjust timeout
                    long adjust = System.nanoTime() - startTime;
-                    to -= TimeUnit.MILLISECONDS.convert(adjust, TimeUnit.NANOSECONDS);
+                    to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);


This will now always assign a negative number to to.

=- is not a compound assignment, it’s negation followed by a normal assignment.

Well spotted, I don't think that change was intentionally.

Ouch; Will fix:

I took Alan's earlier comment literally:

"This one can also be changed to:

to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);"

@RogerRiggs
This already got fixed by @jaikiran in GH‑8693.

8286378: Address possibly lossy conversions in java.base

e72ce85

Add explicit casts to replace implicit casts

openjdk bot added the rfr Pull request is ready for review label May 10, 2022

openjdk bot added security security-dev@openjdk.org nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org i18n i18n-dev@openjdk.org labels May 10, 2022

naotoj approved these changes May 10, 2022

View reviewed changes

src/java.base/share/classes/java/time/es Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label May 10, 2022

XueleiFan approved these changes May 10, 2022

View reviewed changes

bplb approved these changes May 10, 2022

View reviewed changes

remove stray file

7ff806a

AlanBateman reviewed May 11, 2022

View reviewed changes

src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java Outdated Show resolved Hide resolved

AlanBateman reviewed May 11, 2022

View reviewed changes

src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java Show resolved Hide resolved

AlanBateman reviewed May 11, 2022

View reviewed changes

src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java Outdated Show resolved Hide resolved

Updated copyrights

a6679ce

Fixed cast style to add a space after cast, (where consistent with file style) Improved code per review comments in PollSelectors.

bplb approved these changes May 11, 2022

View reviewed changes

AlanBateman approved these changes May 11, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 12, 2022

openjdk bot closed this May 12, 2022

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

ExE-Boss reviewed May 13, 2022

View reviewed changes

jaikiran mentioned this pull request May 13, 2022

8286689: (se) Adjusting to select timeout after EINTR messed up after JDK-8286378 #8693

Closed

3 tasks

RogerRiggs deleted the 8286378-javabase-lossy branch June 10, 2022 20:38

8286378: Address possibly lossy conversions in java.base #8642

8286378: Address possibly lossy conversions in java.base #8642

Uh oh!

Conversation

RogerRiggs commented May 10, 2022 • 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 10, 2022

Uh oh!

openjdk bot commented May 10, 2022

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

XueleiFan left a comment

Choose a reason for hiding this comment

Uh oh!

bplb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asotona commented May 11, 2022

Uh oh!

RogerRiggs commented May 12, 2022

Uh oh!

openjdk bot commented May 12, 2022

Uh oh!

openjdk bot commented May 12, 2022

Uh oh!

ExE-Boss May 13, 2022

Choose a reason for hiding this comment

Uh oh!

AlanBateman May 13, 2022

Choose a reason for hiding this comment

Uh oh!

RogerRiggs May 13, 2022

Choose a reason for hiding this comment

Uh oh!

ExE-Boss May 13, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

RogerRiggs commented May 10, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 10, 2022 •

edited

Loading

openjdk bot commented May 10, 2022 •

edited

Loading