JDK-8297413: Remove easy warnings in javafx.graphics

[hjohn]

• Remove unsupported/unnecessary SuppressWarning annotations
• Remove reduntant type specifications (use diamond operator)
• Remove unused or duplicate imports
• Remove unnecessary casts (type is already correct type or can be autoboxed)
• Remove unnecessary semi-colons (at end of class definitions, or just repeated ones)
• Remove redundant super interfaces (interface that is already inherited)
• Remove unused type parameters
• Remove declared checked exceptions that are never thrown
• Add missing @Override annotations

---

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-8297413: Remove easy warnings in javafx.graphics

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Nir Lisker (@nlisker - Reviewer)
• Andy Goryachev (@andy-goryachev-oracle - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 960

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/960.diff

[bridgekeeper]

👋 Welcome back jhendrikx! 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.

[mlbridge]

Webrevs

• 03: Full - Incremental (a81b9640)
• 02: Full - Incremental (4cc73ecf)
• 01: Full - Incremental (b033ed1c)
• 00: Full (8cb5da22)

[kevinrushforth]

I spot checked the changes. Someone else will need to review it thoroughly.

Overall, I see the following changes as non-controversial:

• removing unused imports
• adding missing @Override
• removing redundant semicolons
• using the diamond operator

The following might need a little more discussion in some cases:

• removing "redundant" casts -- in addition to not always being 100% equivalent (the case where a value is cast to a subclass and later assigned to a superclass), I left a few comments on places where removing redundant floating point casts makes it harder to quickly reason about whether the code is correct.
• removing instanceof (which I didn't see here, but we had discussed in another PR)

As long as the ones in this second list are checked carefully enough, this should be fine, but it isn't as quick a review as it might otherwise be.

[kevinrushforth]

Given the shear number of changes, a second reviewer is in order.

/reviewers 2

[openjdk]

@kevinrushforth
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).

[hjohn]

It's clear that the unnecessary cast removal is causing many discussions, so I think its best to back out that change. The danger in many unnecessary casts is that they can become real casts after a seemingly innocuous change, so it is mostly a benefit to make refactors easier to reason about. Unnecessary casts turning into down casts are the ones that will cause trouble:

    float x, y, z;
    float a = (float)x * (float)y * (float)z;   // unnecessary casts

However, after a refactor:

    double x, y, z;
    float a = (float)x * (float)y * (float)z;   // down casts (no warning!)

The extra precision that one may have hoped to gain is still lost.

Similarly:

    Integer number = ... ;
    Integer x = (Integer) number;  // unnecessary cast

Refactor:

    Number number = ... ;
    Integer x = (Integer) number;  // down cast (no warning!)

The code may now raise a ClassCastException here if Number is not an Integer. Without the unnecessary cast this would be a compile error to be investigated.

[nlisker]

I don't see a problem with removal of the unnecessary casts for numbers.

• It makes the code less cluttered.
• If only necessary casts are left, it draws the attention of the reader to a valuable operation, like rounding with an int cast.
• As John pointed out, if the definition of the types change, it stops being unnecessary and can hide precision loss.
• If I'm at the point where I need to reason about precision of a specific calculation, I can just hover my mouse over the variable and see its type. When just reading the code to understand what it does, the precision/exact types are not important.

In any case, unnecessary casts for non-numeric types should still be removed since the whole precision argument is irrelevant, so I don't think that the warning should be disabled.

[andy-goryachev-oracle]

so I don't think that the warning should be disabled.

It makes no sense to enable the warning and not fix all the warnings in the code. I would like to see 0 warnings in the problem list.

[nlisker]

It makes no sense to enable the warning and not fix all the warnings in the code. I would like to see 0 warnings in the problem list.

The warning is for all unnecessary casts, which removes a lot of clutter. If, in some special cases, the cast is deemed to be beneficial, then it can stay. The warning should not be disabled because it's irrelevant to <1% of the cases. We can maybe fix this with a suppress warning (if one is available).

If you don't want to see the warning, turn it off, but it doesn't mean it shouldn't be addressed.

[nlisker]

Looks good overall. Approval pending decisions on the few outstanding comments.

[kevinrushforth]

There is at least one place where you removed a cast in such a way that the modified code is not equivalent. That change must be reverted before this PR can be accepted. Additionally, I request a change to the time calculation in the gesture recognizer classes.

See inline comments for details.

[kevinrushforth]

Additional inline comments.

[andy-goryachev-oracle]

This is one giant pull request, somewhat difficult on the reviewers, especially since we have to go through it several times.

It would be much easier for the reviewers to deal with one fix per PR, especially in the cases where more than 30 files are going to be touched.

For example, changes involving diamond operator and missing @OVERRIDES could have been made into two separate PRs, making it a breeze to review and integrate, while also cutting down on the number of files which contained non-trivial changes.

What do you think?

[andy-goryachev-oracle]

1 revert and a follow-up bug is requested.

[hjohn]

This is one giant pull request, somewhat difficult on the reviewers, especially since we have to go through it several times.

It would be much easier for the reviewers to deal with one fix per PR, especially in the cases where more than 30 files are going to be touched.

For example, changes involving diamond operator and missing @OVERRIDES could have been made into two separate PRs, making it a breeze to review and integrate, while also cutting down on the number of files which contained non-trivial changes.

What do you think?

Agreed, they'll be smaller and more focused in the future.

[andy-goryachev-oracle]

explanations accepted.

[openjdk]

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

8297413: Remove easy warnings in javafx.graphics

Reviewed-by: kcr, nlisker, angorya

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:

• 7b3c88b: 8295324: JavaFX: Blank pages when printing
• bb98d88: 8295806: TableViewSkin: memory leak when changing skin
• 2c18c18: 8294589: MenuBarSkin: memory leak when changing skin
• c2af0c3: 8238968: Inconsisent formatting of Rectangle2D toString method
• fe81a27: 8287822: [macos] Remove support of duplicated formats from macOS
• 9416874: 8297362: EOS might not be delivered by progressbuffer in some cases
• e64e129: 8295500: AccordionSkin: memory leak when changing skin
• 6ab65a9: 8295506: ButtonBarSkin: memory leak when changing skin
• 4a19fe7: 8295426: MenuButtonSkin: memory leak when changing skin
• 0a785ae: 8295754: PaginationSkin: memory leak when changing skin
• ... and 9 more: https://git.openjdk.org/jfx/compare/a0ea8743eb20bf4791f7a8bf627416aa97c002e5...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @nlisker, @andy-goryachev-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[nlisker]

@hjohn Kevin has outstanding requests. Even though you can technically integrate, you should resolve these with him.

[hjohn]

@hjohn Kevin has outstanding requests. Even though you can technically integrate, you should resolve these with him.

Yes, I will wait until @kevinrushforth had time to take another look.

[hjohn]

/integrate

[openjdk]

@hjohn
Your change (at version a81b964) is now ready to be sponsored by a Committer.

[nlisker]

/sponsor

[openjdk]

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

• 7b3c88b: 8295324: JavaFX: Blank pages when printing
• bb98d88: 8295806: TableViewSkin: memory leak when changing skin
• 2c18c18: 8294589: MenuBarSkin: memory leak when changing skin
• c2af0c3: 8238968: Inconsisent formatting of Rectangle2D toString method
• fe81a27: 8287822: [macos] Remove support of duplicated formats from macOS
• 9416874: 8297362: EOS might not be delivered by progressbuffer in some cases
• e64e129: 8295500: AccordionSkin: memory leak when changing skin
• 6ab65a9: 8295506: ButtonBarSkin: memory leak when changing skin
• 4a19fe7: 8295426: MenuButtonSkin: memory leak when changing skin
• 0a785ae: 8295754: PaginationSkin: memory leak when changing skin
• ... and 9 more: https://git.openjdk.org/jfx/compare/a0ea8743eb20bf4791f7a8bf627416aa97c002e5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@nlisker @hjohn Pushed as commit f333662.

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

[andy-goryachev-oracle]

good job @hjohn !