On Tue, 22 Nov 2022 22:01:10 GMT, Andy Goryachev <ango...@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
