RFR: JDK-8297413: Remove easy warnings in javafx.graphics

[Kevin Rushforth]

On Tue, 22 Nov 2022 22:01:10 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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.
>
> 100.0 is a double, so the result is also a double

Right. Which is why I said that in this specific case it (meaning the changed code) is clear without the explicit cast. So I don't object to this particular change. I just used it as the first example I found to point out an overall concern.

>> 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.
>
> Eclipse knows it is unnecessary.  Personally, I am in favor of removing such unnecessary code.

If it aids readability, I don't see it as unnecessary. In this case, since the types of all of the variables are not known, you could certainly argue it doesn't matter much -- you need to look at the types to reason about what it does.

>> 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).
>
> rotationStartTime is a double.  the type cast is really unnecessary

But you don't know that without looking. Really, though, if I were going to make a readability argument, it's the divisor that should be a double constant, since then there is no question that the division is happen in double.

I don't care too much in this case. I was just making a general point.

>> 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.
>
> good catch - I think the change is not equivalent.  It should be either
> 
> 4L * physicalWidth * physicalHeight;
> 
> 
> or
> 
> 
> ((long)physicalWidth) * physicalHeight * 4L;
> 
> 
> the proposed change might result in overflow bc the first intermediate result is likely to be int.

Actually, I think the new code correct, though less clear, since `(long)` is applied to `physicalWidth` before the multiplication. The fact that we had to scratch our head and discuss it highlights the point I was trying to make. I do think this is one place where reverting the code back would be best. Then it's obvious that all of the intermediate calculations are in long.

>> 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
>
> 4L * w * h;

I would just revert it back.

>> 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.
>
> index_y is a float, so the typecast is unnecessary

Right. In this case I don't really see a problem.

-------------

PR: https://git.openjdk.org/jfx/pull/960