Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 15:07:49 GMT, Nir Lisker wrote: >> The array is of type Object. When LHS is Object then autobox chooses a >> suitable type by itself. >> So here, if we autobox then the values get autoboxed to `Integer`. We do not >> intend to change the test behavior as part of this fix so let's keep this as >> `Double.valueOf`. > > I meant that we could use a double literal: `503d` or `503.0`, but if you > think this is more readable then it's fine as is. Thanks, I would like to keep as is. - PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 15:18:51 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/J2DPipeline.java
>> line 64:
>>
>>> 62: @Override
>>> 63: public ResourceFactory getResourceFactory(Screen screen) {
>>> 64: Integer index = Integer.valueOf(screen.getAdapterOrdinal());
>>
>> Autobox?
>>
>> Same for the other similar places.
>
> Do you have a preference here?
I am little more on the side to use explicit calls when using values returned
by a method. The method `screen.getAdapterOrdinal()` returns an int. But if it
returned short then autoboxing will fail, but `Integer.valueOf()` would still
work. I don't think the method would ever change, but I still lean on using
explicit call. Being explicit safe guards more and anyway both still create an
object.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 07:34:29 GMT, Ambarish Rapte wrote:
>> modules/javafx.graphics/src/main/java/com/sun/prism/es2/X11GLFactory.java
>> line 171:
>>
>>> 169: deviceDetails.put("XVisualID",
>>> Long.valueOf(nGetVisualID(nativeCtxInfo)));
>>> 170: deviceDetails.put("XDisplay",
>>> Long.valueOf(nGetDisplay(nativeCtxInfo)));
>>> 171: deviceDetails.put("XScreenID",
>>> Integer.valueOf(nGetDefaultScreen(nativeCtxInfo)));
>>
>> Autobox?
>
> I think the statements look more readable the way they are.
> The type of LHS of the expression is Object, so a reader will have to find
> return type of those methods to understand what type of Object gets created.
> I think such calls which use a return value from a method should use explicit
> calls.
> But then on other side in case if return type of the method is changed, then
> we need to also change these explicit calls.
> Sounds like a decision to make for other time. What do you think ?
The real problem here is not finding out what the return type is, it's that the
`deviceDetails` type is a raw `HashMap` so it's not clear what type should be
put in the map. If it were `HashMap` then the reader should not
care what gets put into the map since it's an `Object`.
It's fine as is anyway.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:31:10 GMT, Nir Lisker wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> correct Float.valueOf()
>
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/J2DPipeline.java line
> 64:
>
>> 62: @Override
>> 63: public ResourceFactory getResourceFactory(Screen screen) {
>> 64: Integer index = Integer.valueOf(screen.getAdapterOrdinal());
>
> Autobox?
>
> Same for the other similar places.
Do you have a preference here?
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 06:01:04 GMT, Ambarish Rapte wrote:
>> apps/samples/Ensemble8/src/samples/java/ensemble/samples/language/swing/SampleTableModel.java
>> line 54:
>>
>>> 52: {Double.valueOf(567), Double.valueOf(956),
>>> Double.valueOf(1154)},
>>> 53: {Double.valueOf(1292), Double.valueOf(1665),
>>> Double.valueOf(1927)},
>>> 54: {Double.valueOf(1292), Double.valueOf(2559),
>>> Double.valueOf(2774)}
>>
>> Autobox?
>
> The array is of type Object. When LHS is Object then autobox chooses a
> suitable type by itself.
> So here, if we autobox then the values get autoboxed to `Integer`. We do not
> intend to change the test behavior as part of this fix so let's keep this as
> `Double.valueOf`.
I meant that we could use a double literal: `503d` or `503.0`, but if you think
this is more readable then it's fine as is.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 05:47:31 GMT, Ambarish Rapte wrote:
>> modules/javafx.web/src/ios/java/javafx/scene/web/JSONDecoder.java line 101:
>>
>>> 99: long val = Long.parseLong(sNum);
>>> 100: if ((val <= Integer.MAX_VALUE) && (Integer.MIN_VALUE <=
>>> val)) {
>>> 101: return Integer.valueOf(int) val);
>>
>> Autobox?
>
> I think, when LHS is of type `Object`, it is good to be explicit on creating
> Object, it improves readability.
> In this case, type of val is `long`, so autobox would create an object of
> `Long` not `Integer`.
RIght, this can lead to bugs if you aren't careful. See the discussion in
[JDK-8154213](https://bugs.openjdk.java.net/browse/JDK-8154213). I recommend
autoboxing only for those cases where the type is clearly specified.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:29:41 GMT, Nir Lisker wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> correct Float.valueOf()
>
> modules/javafx.graphics/src/main/java/com/sun/prism/es2/X11GLFactory.java
> line 171:
>
>> 169: deviceDetails.put("XVisualID",
>> Long.valueOf(nGetVisualID(nativeCtxInfo)));
>> 170: deviceDetails.put("XDisplay",
>> Long.valueOf(nGetDisplay(nativeCtxInfo)));
>> 171: deviceDetails.put("XScreenID",
>> Integer.valueOf(nGetDefaultScreen(nativeCtxInfo)));
>
> Autobox?
I think the statements look more readable the way they are.
The type of LHS of the expression is Object, so a reader will have to find
return type of those methods to understand what type of Object gets created. I
think such calls which use a return value from a method should use explicit
calls.
But then on other side in case if return type of the method is changed, then we
need to also change these explicit calls.
Sounds like a decision to make for other time. What do you think ?
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Wed, 10 Mar 2021 23:59:05 GMT, Nir Lisker wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> correct Float.valueOf() > > apps/toys/LayoutDemo/src/layout/CustomPane.java line 92: > >> 90: Collections.sort(sortedManagedChidlren, (c1, c2) >> 91: -> Double.valueOf(c2.prefHeight(-1)).compareTo( >> 92: Double.valueOf(c1.prefHeight(-1; > > This can be replaced with > > Collections.sort(sortedManagedChidlren, (c1, c2) -> > Double.compare(c2.prefHeight(-1), c1.prefHeight(-1))); > > or > > Collections.sort(sortedManagedChidlren, > Comparator.comparingDouble(n -> n.prefHeight(-1)).reversed()); > > I think. > Same for the other places that do comparing. As this change is a cleanup and intended to replace deprecated constructor calls with autoboxing or `valueOf()` method, I think we should not make other changes as part of this PR. - PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:18:17 GMT, Nir Lisker wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> correct Float.valueOf()
>
> modules/javafx.base/src/test/java/test/javafx/util/DurationTest.java line 289:
>
>> 287: @Test public void add_ZERO_and_INDEFINITE_ResultsInIndefinite() {
>> 288: //assertTrue(0.0 + Double.POSITIVE_INFINITY ==
>> Double.POSITIVE_INFINITY); // sanity check
>> 289: assertEquals(Double.valueOf(Double.POSITIVE_INFINITY),
>> Double.valueOf(0.0 + Double.POSITIVE_INFINITY)); // sanity check
>
> I don't understand why convert to `Double` for the assertion test, but more
> than that, I don't understand why this test is needed for `Duration`. Isn't
> this is a guaranteed behavior of fp arithmetic?
>
> Same for all other such asserts.
>From the look of these tests, many look unnecessary to be here. But As these
>tests exist from very beginning, there might have been a good reason that they
>were added. As long as they don't harm, and if you don't have any hard call on
>them, let's keep them as is.(with just the `valueOf` change)
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:23:48 GMT, Nir Lisker wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> correct Float.valueOf() > > modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYChartTest.java > line 85: > >> 83: Font f = yaxis.getTickLabelFont(); >> 84: // default caspian value for font size = 10 >> 85: assertEquals(10, Double.valueOf(f.getSize()).intValue()); > > Any reason to convert `f.getSize()` to `int` and then to `Double`? Is it for > flooring the `double`? > > Same for the other places. `f.getSize()` returns a double value, the statement can also be written as `assertEquals(10, f.getSize(), 0f);` But, as this change is a cleanup and is intended to remove the calls to deprecated constructors, I think it should be limited to changing calls to autobox or to use `valueOf()` method. - PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:12:25 GMT, Nir Lisker wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> correct Float.valueOf()
>
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/MappingChangeTest.java
> line 52:
>
>> 50: @Test
>> 51: public void testAddRemove() {
>> 52: Change change = new
>> NonIterableChange.SimpleRemovedChange(0, 1, Integer.valueOf(5),
>> originalList);
>
> Autobox?
First two parameters are primitive type integer and the third parameter is
template type.
Changing it to autobox would make it less readable.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:26:38 GMT, Nir Lisker wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> correct Float.valueOf()
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
> line 643:
>
>> 641: ComboBox cb = new ComboBox();
>> 642: StringConverter sc = cb.getConverter();
>> 643: assertEquals("42", sc.toString(Integer.valueOf(42)));
>
> Autobox?
>From this test code the type of parameter of `sc.toString()` is not obvious.
`StringConverter` is a template class. `toString()` accepts a template type
parameter.
Using autoboxing at such instances would hamper the readability.
So I think autoboxing should be used only when the type is very obvious from
just a glance of code.
> apps/samples/Ensemble8/src/samples/java/ensemble/samples/language/swing/SampleTableModel.java
> line 54:
>
>> 52: {Double.valueOf(567), Double.valueOf(956), Double.valueOf(1154)},
>> 53: {Double.valueOf(1292), Double.valueOf(1665),
>> Double.valueOf(1927)},
>> 54: {Double.valueOf(1292), Double.valueOf(2559),
>> Double.valueOf(2774)}
>
> Autobox?
The array is of type Object. When LHS is Object then autobox chooses a suitable
type by itself.
So here, if we autobox then the values get autoboxed to `Integer`. We do not
intend to change the test behavior as part of this fix so let's keep this as
`Double.valueOf`.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Thu, 11 Mar 2021 00:41:48 GMT, Nir Lisker wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> correct Float.valueOf()
>
> modules/javafx.web/src/ios/java/javafx/scene/web/JSONDecoder.java line 101:
>
>> 99: long val = Long.parseLong(sNum);
>> 100: if ((val <= Integer.MAX_VALUE) && (Integer.MIN_VALUE <=
>> val)) {
>> 101: return Integer.valueOf(int) val);
>
> Autobox?
I think, when LHS is of type `Object`, it is good to be explicit on creating
Object, it improves readability.
In this case, type of val is `long`, so autobox would create an object of
`Long` not `Integer`.
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Wed, 10 Mar 2021 19:49:25 GMT, Ambarish Rapte wrote:
>> The following primitive constructors were deprecated in JDK 9 and are
>> deprecated for removal in JDK 16.
>>
>> java.lang.Byte
>> java.lang.Short
>> java.lang.Integer
>> java.lang.Long
>> java.lang.Float
>> java.lang.Double
>> java.lang.Character
>> java.lang.Boolean
>>
>> This change removes call to the primitive constructors with the `valueOf()`
>> factory method of respective class.
>> Calls like the following to create array get autoboxed so it does not
>> require a change.
>>
>> `Double dArr[] = new Double[] {10.1, 20.2};`
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> correct Float.valueOf()
I left some comments where I think autoboxing does the same work explicit
boxing does. Unless I'm missing something, all these places can be simplified.
apps/samples/Ensemble8/src/app/java/ensemble/samplepage/PieChartDataVisualizer.java
line 92:
> 90: }
> 91: try {
> 92: return (Double) Double.valueOf(string);
Looks like an unnecessary cast.
apps/toys/LayoutDemo/src/layout/CustomPane.java line 92:
> 90: Collections.sort(sortedManagedChidlren, (c1, c2)
> 91: -> Double.valueOf(c2.prefHeight(-1)).compareTo(
> 92: Double.valueOf(c1.prefHeight(-1;
This can be replaced with
Collections.sort(sortedManagedChidlren, (c1, c2) ->
Double.compare(c2.prefHeight(-1), c1.prefHeight(-1)));
or
Collections.sort(sortedManagedChidlren, Comparator.comparingDouble(n
-> n.prefHeight(-1)).reversed());
I think.
Same for the other places that do comparing.
modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java
line 582:
> 580:
> 581: boolean click = true;
> 582: final Boolean firstClick = Boolean.valueOf(click);
Autobox?
apps/samples/3DViewer/src/main/java/com/javafx/experiments/shape3d/SkinningMesh.java
line 110:
> 108: for (int i = 0; i < nPoints; i++) {
> 109: if (weights[j][i] != 0.0f) {
> 110: weightIndices[j].add(Integer.valueOf(i));
Autobox?
apps/samples/Ensemble8/src/samples/java/ensemble/samples/language/swing/SampleTableModel.java
line 54:
> 52: {Double.valueOf(567), Double.valueOf(956), Double.valueOf(1154)},
> 53: {Double.valueOf(1292), Double.valueOf(1665),
> Double.valueOf(1927)},
> 54: {Double.valueOf(1292), Double.valueOf(2559), Double.valueOf(2774)}
Autobox?
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/MappingChangeTest.java
line 52:
> 50: @Test
> 51: public void testAddRemove() {
> 52: Change change = new
> NonIterableChange.SimpleRemovedChange(0, 1, Integer.valueOf(5),
> originalList);
Autobox?
modules/javafx.base/src/test/java/test/javafx/util/DurationTest.java line 289:
> 287: @Test public void add_ZERO_and_INDEFINITE_ResultsInIndefinite() {
> 288: //assertTrue(0.0 + Double.POSITIVE_INFINITY ==
> Double.POSITIVE_INFINITY); // sanity check
> 289: assertEquals(Double.valueOf(Double.POSITIVE_INFINITY),
> Double.valueOf(0.0 + Double.POSITIVE_INFINITY)); // sanity check
I don't understand why convert to `Double` for the assertion test, but more
than that, I don't understand why this test is needed for `Duration`. Isn't
this is a guaranteed behavior of fp arithmetic?
Same for all other such asserts.
modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYChartTest.java
line 85:
> 83: Font f = yaxis.getTickLabelFont();
> 84: // default caspian value for font size = 10
> 85: assertEquals(10, Double.valueOf(f.getSize()).intValue());
Any reason to convert `f.getSize()` to `int` and then to `Double`? Is it for
flooring the `double`?
Same for the other places.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
line 643:
> 641: ComboBox cb = new ComboBox();
> 642: StringConverter sc = cb.getConverter();
> 643: assertEquals("42", sc.toString(Integer.valueOf(42)));
Autobox?
modules/javafx.graphics/src/main/java/com/sun/prism/es2/X11GLFactory.java line
171:
> 169: deviceDetails.put("XVisualID",
> Long.valueOf(nGetVisualID(nativeCtxInfo)));
> 170: deviceDetails.put("XDisplay",
> Long.valueOf(nGetDisplay(nativeCtxInfo)));
> 171: deviceDetails.put("XScreenID",
> Integer.valueOf(nGetDefaultScreen(nativeCtxInfo)));
Autobox?
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/J2DPipeline.java line
64:
> 62: @Override
> 63: public ResourceFactory getResourceFactory(Screen screen) {
> 64: Integer index = Integer.valueOf(screen.getAdapterOrdinal());
Autobox?
Same for the other similar places.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 477:
> 475: v.set(value);
> 476:
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Wed, 10 Mar 2021 19:49:25 GMT, Ambarish Rapte wrote:
>> The following primitive constructors were deprecated in JDK 9 and are
>> deprecated for removal in JDK 16.
>>
>> java.lang.Byte
>> java.lang.Short
>> java.lang.Integer
>> java.lang.Long
>> java.lang.Float
>> java.lang.Double
>> java.lang.Character
>> java.lang.Boolean
>>
>> This change removes call to the primitive constructors with the `valueOf()`
>> factory method of respective class.
>> Calls like the following to create array get autoboxed so it does not
>> require a change.
>>
>> `Double dArr[] = new Double[] {10.1, 20.2};`
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> correct Float.valueOf()
Marked as reviewed by kcr (Lead).
-
PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
On Wed, 10 Mar 2021 19:37:51 GMT, Kevin Rushforth wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> correct Float.valueOf() > > Looks good with a couple problems (in code that we must not be building) > noted below. This is a simple change, but it would be good to have a second pair of eyes on it. - PR: https://git.openjdk.java.net/jfx/pull/423
Re: RFR: 8257512: Remove use of deprecated primitive constructors in JavaFX [v3]
> The following primitive constructors were deprecated in JDK 9 and are
> deprecated for removal in JDK 16.
>
> java.lang.Byte
> java.lang.Short
> java.lang.Integer
> java.lang.Long
> java.lang.Float
> java.lang.Double
> java.lang.Character
> java.lang.Boolean
>
> This change removes call to the primitive constructors with the `valueOf()`
> factory method of respective class.
> Calls like the following to create array get autoboxed so it does not require
> a change.
>
> `Double dArr[] = new Double[] {10.1, 20.2};`
Ambarish Rapte has updated the pull request incrementally with one additional
commit since the last revision:
correct Float.valueOf()
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/423/files
- new: https://git.openjdk.java.net/jfx/pull/423/files/588ec4ce..e34c6a8f
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=423=02
- incr: https://webrevs.openjdk.java.net/?repo=jfx=423=01-02
Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
Patch: https://git.openjdk.java.net/jfx/pull/423.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/423/head:pull/423
PR: https://git.openjdk.java.net/jfx/pull/423
