Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Wed, 30 Nov 2022 12:48:10 GMT, Kevin Rushforth wrote: >> If the casts in the numerator actually matter, then the cast in the >> denominator can be removed. The latter are the ones that Eclipse flags for >> me as unnecessary. > > I still think that any change here would be a very low value change. If done > incorrectly, as it was in the initial attempt, it can introduce bugs. Even if > done correctly, I see no point in it. @kevinrushforth This has been mostly reverted, and is a lesson for me to not do more than what the warning suggests without checking very carefully. It just looked like a bunch of unnecessary casts, but it is has one edge case with negation overflow of a signed integer that I missed (whether it can happen in practice I did not analyze further). - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Wed, 30 Nov 2022 12:48:10 GMT, Kevin Rushforth wrote: >> If the casts in the numerator actually matter, then the cast in the >> denominator can be removed. The latter are the ones that Eclipse flags for >> me as unnecessary. > > I still think that any change here would be a very low value change. If done > incorrectly, as it was in the initial attempt, it can introduce bugs. Even if > done correctly, I see no point in it. I don't really mind either way. Eclipse's refactoring are safe, so I don't see any risk in doing what it says (removing the denominator casts). For me, the less casts the more clear what the calculation is doing. Like I said before, it's probably a personal thing. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Tue, 29 Nov 2022 16:50:00 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix indentation > > modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java > line 306: > >> 304: sendRotateEvent(false); >> 305: angleReference = newAngle; >> 306: double timePassed = (time - rotationStartTime) >> / 10; > > Ditto here and in the other gesture recognizers. I've reverted these and filed the follow up bug https://bugs.openjdk.org/browse/JDK-8298060 - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Tue, 22 Nov 2022 21:28:44 GMT, Kevin Rushforth wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix indentation
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/UploadingPainter.java
> line 41:
>
>> 39: * The PresentingPainter is used when we are rendering to the main
>> screen.
>> 40: */
>> 41: final class UploadingPainter extends ViewPainter {
>
> I'm not sure I see the value in making this change.
Since this is an internal class (not public API), I won't object to this
change. I do think it's a rather pointless change, but I'll leave it up to you.
-
PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Mon, 28 Nov 2022 13:39:05 GMT, John Hendrikx wrote: >> - 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 > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix indentation Additional inline comments. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Wed, 30 Nov 2022 03:19:54 GMT, Nir Lisker wrote: >> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java >> line 418: >> >>> 416: scaleFactor = 1.0 / scaleDivider; >>> 417: adjw = (int)Math.round(iw / scaleDivider); >>> 418: adjh = (int)Math.round(ih / scaleDivider); >> >> Same comment here about the old code being clearer. > > `scaleDivider` is defined just 2 lines above as a `double`. I don't see how > the cast helps here. Yes, this one is fine. >> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java >> line 422: >> >>> 420: } >>> 421: double similarity = (width - adjw) / (double) width + >>> 422: (height - adjh) / (double) height + //Large >>> padding is bad >> >> This change _must_ be reverted. There are cases where doing integer >> arithmetic on intermediate results is not equivalent to doing double >> arithmetic. >> >> Consider this: >> >> >> int i = Integer.MAX_VALUE; >> int j = -1; >> double d1 = (double) i - (double) j; >> double d2 = i - j; >> >> >> `d1` will be `2147483648` and `d2` will be `-2147483648`. >> >> I can't say whether it is possible in practice, but this change is not >> acceptable, and is a good example of the general concern I raised. > > If the casts in the numerator actually matter, then the cast in the > denominator can be removed. The latter are the ones that Eclipse flags for me > as unnecessary. I still think that any change here would be a very low value change. If done incorrectly, as it was in the initial attempt, it can introduce bugs. Even if done correctly, I see no point in it. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Tue, 22 Nov 2022 21:30:09 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix indentation > > modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java > line 418: > >> 416: scaleFactor = 1.0 / scaleDivider; >> 417: adjw = (int)Math.round(iw / scaleDivider); >> 418: adjh = (int)Math.round(ih / scaleDivider); > > Same comment here about the old code being clearer. `scaleDivider` is defined just 2 lines above as a `double`. I don't see how the cast helps here. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Tue, 29 Nov 2022 17:11:51 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix indentation > > modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java > line 422: > >> 420: } >> 421: double similarity = (width - adjw) / (double) width + >> 422: (height - adjh) / (double) height + //Large padding >> is bad > > This change _must_ be reverted. There are cases where doing integer > arithmetic on intermediate results is not equivalent to doing double > arithmetic. > > Consider this: > > > int i = Integer.MAX_VALUE; > int j = -1; > double d1 = (double) i - (double) j; > double d2 = i - j; > > > `d1` will be `2147483648` and `d2` will be `-2147483648`. > > I can't say whether it is possible in practice, but this change is not > acceptable, and is a good example of the general concern I raised. If the casts in the numerator actually matter, then the cast in the denominator can be removed. The latter are the ones that Eclipse flags for me as unnecessary. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Wed, 23 Nov 2022 16:49:50 GMT, Andy Goryachev wrote: >> You are correct that with these kind of changes you can't see if it is >> correct just from looking at the diff. Variables would need to be named more >> explicitly, or explicit casts would need to be added to repeat the type >> information. If the goal is to make it clear in the diff, then many places >> would benefit from explicit casts (probably in more places than where they >> were removed). I think the general trend is to avoid explicit casts, but I >> could be wrong. >> >> I can revert these changes and we can disable the warning. >> >> Specifically about this code, `rotationStartTime` probably should not have >> been a `double` depending on what is stored in it (checking: its source is >> `System.nanoTime()`) as the conversion from `long` to `double` will lose >> precision in the area where it really counts as it wants to see the >> difference of the current and last value, so in this case the warning may >> have exposed a (precision) bug. > > +1 for reverting the changes and disabling the warning > Specifically about this code, `rotationStartTime` probably should not have > been a `double` depending on what is stored in it (checking: its source is > `System.nanoTime()`) as the conversion from `long` to `double` will lose > precision in the area where it really counts as it wants to see the > difference of the current and last value, so in this case the warning may > have exposed a (precision) bug. You may be right. In this case, I would recommend either reverting this change, or else adding a `.0` to the divisor along with the removal of the explicit cast. Either way, we should consider a follow-up bug to address the question of possible loss of precision (in the existing code, which is unchanged by the refactoring). - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Tue, 22 Nov 2022 21:34:44 GMT, Kevin Rushforth wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix indentation
>
> modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2VramPool.java line
> 54:
>
>> 52: {
>> 53: // REMIND: need to deal with size of depth buffer, etc.
>> 54: return (long) width * height * 4;
>
> DItto
As with the other occurrences, `4L * width * height` would be clearer.
-
PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Mon, 28 Nov 2022 13:39:05 GMT, John Hendrikx wrote: >> - 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 > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix indentation There is at least one place where you removed a cast in such a way that the modified code is not equivalent. That change that _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. modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java line 306: > 304: sendRotateEvent(false); > 305: angleReference = newAngle; > 306: double timePassed = (time - rotationStartTime) / > 10; Ditto here and in the other gesture recognizers. modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java line 422: > 420: } > 421: double similarity = (width - adjw) / (double) width + > 422: (height - adjh) / (double) height + //Large padding > is bad This change _must_ be reverted. There are cases where doing integer arithmetic on intermediate results is not equivalent to doing double arithmetic. Consider this: int i = Integer.MAX_VALUE; int j = -1; double d1 = (double) i - (double) j; double d2 = i - j; `d1` will be `2147483647` and `d2` will be `-2147483647`. I can't say whether it is possible in practice, but this change is not acceptable, and is a good example of the general concern I raised. - Changes requested by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Sat, 26 Nov 2022 19:24:21 GMT, Nir Lisker wrote: >> The cast has precedence and Java will not downcast mid calculation and lose >> precision, so all examples here are correct. > > `4L * physicalWidth * physicalHeight;` does not require an explicit cast, so > I think it's the clearest. That variant is fine with me. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
> - 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 John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Fix indentation - Changes: - all: https://git.openjdk.org/jfx/pull/960/files - new: https://git.openjdk.org/jfx/pull/960/files/8cb5da22..b033ed1c Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=960=01 - incr: https://webrevs.openjdk.org/?repo=jfx=960=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/960.diff Fetch: git fetch https://git.openjdk.org/jfx pull/960/head:pull/960 PR: https://git.openjdk.org/jfx/pull/960
