On Tue, 22 Nov 2022 18:39:43 GMT, John Hendrikx <[email protected]> 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
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.
Given the shear number of changes, a second reviewer is in order.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkMenuDelegate.java
line 52:
> 50:
> 51: @Override
> 52: public boolean setPixels(Pixels pixels) {
Indentation was lost here.
modules/javafx.graphics/src/main/java/com/sun/javafx/application/LauncherImpl.java
line 783:
> 781: if (simulateSlowProgress) {
> 782: for (int i = 0; i < 100; i++) {
> 783: notifyProgress(currentPreloader, i / 100.0);
Does Eclipse really flag this as a warning? It doesn't seem a particularly
useful warning, since an explicit cast to double is a reasonable pattern that
can improve clarity, even if it isn't needed. In this specific case it is
pretty clear without the explicit cast that it will do what you want, but other
places are not so clear.
modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java
line 1240:
> 1238: cadv = advanceWidths[numHMetrics-1];
> 1239: }
> 1240: return ((cadv & 0xffff)*ptSize)/upem;
This is an example where I think the explicit cast makes it easier to quickly
tell at a glance that it is correct, even though it happens to be unnecessary.
In this case, I would need to look at the definition of `ptSize` and `upem` to
know that it is unneeded, and thus correct without it.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java
line 230:
> 228: }
> 229: if (ROTATION_INERTIA_ENABLED && (state ==
> RotateRecognitionState.PRE_INERTIA || state ==
> RotateRecognitionState.ACTIVE)) {
> 230: double timeFromLastRotation = (time - rotationStartTime)
> / 1000000;
Another place where I'm not sure removing the explicit cast is best (probably
OK here).
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.
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.
modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DTextureData.java
line 45:
> 43: boolean hasDepth)
> 44: {
> 45: return (long) physicalWidth * physicalHeight * 4L;
Another case where the old code might be clearer.
modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DVramPool.java line
59:
> 57: {
> 58: // REMIND: need to deal with size of depth buffer, etc.
> 59: return (long) width * height * 4;
Ditto
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
modules/javafx.graphics/src/main/java/com/sun/prism/impl/ps/PaintHelper.java
line 310:
> 308: shader.setConstants("fractions", stopVals, 0,
> MULTI_MAX_FRACTIONS);
> 309: float index_y = initGradient(paint);
> 310: shader.setConstant("offset", index_y / MULTI_CACHE_SIZE +
> HALF_TEXEL_Y);
Here's another.
-------------
PR: https://git.openjdk.org/jfx/pull/960
