Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Mon, 29 Jan 2024 09:09:40 GMT, Karthik P K wrote:
>> Laurent Bourgès has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fixed test package
>
> modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheDouble.java
> line 168:
>
>> 166: if (DO_CLEAN_DIRTY && (toIndex != 0)) {
>> 167: // clean-up array of dirty part[fromIndex; toIndex[
>> 168: fill(array, fromIndex, toIndex, /*(double)*/ 0.0);
>
> Do you think /*(double)*/ will be helpful? In my opinion, it is not required.
@bourgesl are planning to remove these comments? Wanted to check since the
comment was resolved without change.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1479636490
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Mon, 29 Jan 2024 09:44:04 GMT, Karthik P K wrote: >> Laurent Bourgès has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fixed test package > > modules/javafx.graphics/src/main/java/com/sun/marlin/DPathConsumer2D.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2007, 2022, Oracle and/or its affiliates. All rights >> reserved. > > Shouldn't it be 2007, 2024? In general it is not required to update the copyright year along with the fix, though it is not a hard rule. We update the years for all files before release. But as you are modifying please change this one to 24. - PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1474210526
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Thu, 25 Jan 2024 17:16:51 GMT, Laurent Bourgès wrote: >> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > fixed test package Our headful CI test run passed. - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1915252859
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Fri, 26 Jan 2024 06:49:42 GMT, Laurent Bourgès wrote: > I fixed new test so system test run properly. How do you compare test runs in > CI ? GHA jobs only run headless tests. In order to do run system test we need physical test systems. We have the ability to run them internally (in our lab), so I'll do a test run and post results. - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1914777538
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Thu, 25 Jan 2024 17:16:51 GMT, Laurent Bourgès wrote:
>> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java
>
> Laurent Bourgès has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fixed test package
Tested the changes and it fixes the issue as expected.
Left some minor comments inline. I will finish the review soon.
modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheDouble.java line
168:
> 166: if (DO_CLEAN_DIRTY && (toIndex != 0)) {
> 167: // clean-up array of dirty part[fromIndex; toIndex[
> 168: fill(array, fromIndex, toIndex, /*(double)*/ 0.0);
Do you think /*(double)*/ will be helpful? In my opinion, it is not required.
modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheInt.java line
168:
> 166: if (DO_CLEAN_DIRTY && (toIndex != 0)) {
> 167: // clean-up array of dirty part[fromIndex; toIndex[
> 168: fill(array, fromIndex, toIndex, /*(int)*/ 0);
Same as previous comment, /*(int)*/ comment is not required here.
modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheIntClean.java
line 171:
> 169: if (toIndex != 0) {
> 170: // clean-up array of dirty part[fromIndex; toIndex[
> 171: fill(array, fromIndex, toIndex, /*(int)*/ 0);
Same as previous comment, /*(int)*/ comment is not required here.
modules/javafx.graphics/src/main/java/com/sun/marlin/DPathConsumer2D.java line
2:
> 1: /*
> 2: * Copyright (c) 2007, 2022, Oracle and/or its affiliates. All rights
> reserved.
Shouldn't it be 2007, 2024?
modules/javafx.graphics/src/main/java/com/sun/prism/impl/shape/DMarlinPrismUtils.java
line 111:
> 109:
> 110: if (Math.abs(det) <= (2.0d * Double.MIN_VALUE)) {
> 111: // this rendering engine takes one dimensional curves
> and turns
Minor: this -> This
modules/javafx.graphics/src/main/java/com/sun/prism/impl/shape/DMarlinPrismUtils.java
line 118:
> 116:
> 117: // Every path needs an initial moveTo and a pathDone. If
> these
> 118: // are not there this causes a SIGSEGV in libawt.so (at
> the time
Minor: Add space before so and S can be made capital.
Also wanted to check if libawt usage is correct here?
-
PR Review: https://git.openjdk.org/jfx/pull/1348#pullrequestreview-1848155818
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469276629
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469277966
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469279357
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469322152
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469352123
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469351817
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Thu, 25 Jan 2024 17:16:51 GMT, Laurent Bourgès wrote: >> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > fixed test package I fixed new test so system test run properly. How do you compare test runs in CI ? - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1911570665
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java Laurent Bourgès has updated the pull request incrementally with one additional commit since the last revision: fixed test package - Changes: - all: https://git.openjdk.org/jfx/pull/1348/files - new: https://git.openjdk.org/jfx/pull/1348/files/91061b8a..251d1d61 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1348&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1348&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1348.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1348/head:pull/1348 PR: https://git.openjdk.org/jfx/pull/1348
