Re: RFR: 8088923: IOOBE when adding duplicate categories to the BarChart [v5]
On Sat, 22 Jun 2024 10:55:28 GMT, Markus Mack wrote: >> This PR provides the test case given in the JBS issue, and a simple fix for >> the index calculation when inserting data after previous data with duplicate >> categories. >> >> Also, I've added a comment to `BarChart`s javadoc, clarifying the behavior >> that was apparently assumed (but broken) previously. >> >> The index lookup is skipped for performance reasons if there are no >> duplicates, corresponding to the previous implementation. >> Further optimizations would be possible, but probably are not really helpful >> without more extensive changes. The previous code already loops over all >> categories to check if they are present, typically nested in a loop adding >> many data items, thus already scaling quadratically when adding lots of >> mostly unique data points. > > Markus Mack has updated the pull request incrementally with one additional > commit since the last revision: > > add javadoc clarification Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1476#pullrequestreview-2133846362
Re: RFR: 8327255: javac lint warnings: removal, missing-explicit-ctor [v2]
On Tue, 18 Jun 2024 14:16:25 GMT, Ambarish Rapte wrote:
>> Kevin Rushforth has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'upstream/master' into
>> 8327255-lint-removal,missing-explicit-ctor
>> - fix typo and add missing tool tasks
>> - 8334138: Suppress removal warnings for sun.misc.Unsafe memory access
>> methods
>> - 8334143: Suppress remaining removal warnings for internal methods and
>> classes
>> - 8327255: javac lint warnings: removal, missing-explicit-ctor
>
> build.gradle line 4139:
>
>> 4137: }
>> 4138: } else if (compile.name == "compileJslcJava" ||
>> 4139: compile.name == "compileToolJava") {
>
> Should we include the _compileToolsJava_ task from media too ?
That was a typo: "compileToolJava" should have been "compileToolsJava". While
checking this, I also found that I missed two other tools tasks,
"compileDecoraCompilers" and "compilePrismCompilers", so I added them as well.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1474#discussion_r1649699902
Re: RFR: 8327255: javac lint warnings: removal, missing-explicit-ctor [v2]
> This PR updates `build.gradle` to define javac lint options for three > different types of java compilation tasks: sdk classes, test classes > (including shims), and tool classes (including JSLC). The defaults for these > three groups of lint options are specified in `build.gradle`. > > We also define three gradle properties that can be used at build time to > override the global lint options for each of the three categories of tasks as > follows: > > * `LINT` - used when compiling sdk tasks : default = > "removal,missing-explicit-ctor" > * `TOOL_LINT` - used when compiling tool tasks : default = "" > * `TEST_LINT` - used when compiling test tasks : default = "" > > Each property takes a string with a comma-separated list of lint options. For > example: > > > gradle -PLINT="deprecation,removal" sdk > > > I provided a build mechanism to add extra per-module lint options in > `build.gradle`. None are currently defined, but I tested the logic to show > that it can be. I did not provide a way to override the per-module lint > options via the command line. Once we start using them, we might consider > adding that. > > While testing this PR, I ran into a few places where we need to suppress > removal warnings for use of our own internal classes that are terminally > deprecated, so I filed > [JDK-8334143](https://bugs.openjdk.org/browse/JDK-8334143) to track this, and > added the missing annotations in this PR. > > I also tested with JDK 23 to verify that we are now getting the expected > warnings for the use of sun.misc.Unsafe, and filed two bugs > --[JDK-8334138](https://bugs.openjdk.org/browse/JDK-8334138) to (temporarily) > suppress those warnings in this PR and > [JDK-8334137](https://bugs.openjdk.org/browse/JDK-8334137) to eventually fix > them by replacing our use of sun.misc.Unsafe. Kevin Rushforth has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8327255-lint-removal,missing-explicit-ctor - fix typo and add missing tool tasks - 8334138: Suppress removal warnings for sun.misc.Unsafe memory access methods - 8334143: Suppress remaining removal warnings for internal methods and classes - 8327255: javac lint warnings: removal, missing-explicit-ctor - Changes: - all: https://git.openjdk.org/jfx/pull/1474/files - new: https://git.openjdk.org/jfx/pull/1474/files/1d90c796..753d5377 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1474&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1474&range=00-01 Stats: 5235 lines in 61 files changed: 5136 ins; 36 del; 63 mod Patch: https://git.openjdk.org/jfx/pull/1474.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1474/head:pull/1474 PR: https://git.openjdk.org/jfx/pull/1474
Re: RFR: 8088923: IOOBE when adding duplicate categories to the BarChart [v5]
On Sat, 22 Jun 2024 10:55:28 GMT, Markus Mack wrote: >> This PR provides the test case given in the JBS issue, and a simple fix for >> the index calculation when inserting data after previous data with duplicate >> categories. >> >> Also, I've added a comment to `BarChart`s javadoc, clarifying the behavior >> that was apparently assumed (but broken) previously. >> >> The index lookup is skipped for performance reasons if there are no >> duplicates, corresponding to the previous implementation. >> Further optimizations would be possible, but probably are not really helpful >> without more extensive changes. The previous code already loops over all >> categories to check if they are present, typically nested in a loop adding >> many data items, thus already scaling quadratically when adding lots of >> mostly unique data points. > > Markus Mack has updated the pull request incrementally with one additional > commit since the last revision: > > add javadoc clarification I can reproduce this, looks unrelated to the issue this PR is trying to fix. The category axis entries look reasonable, while the calculated plot coordinates are off. Not sure if this is the same as [8314754](https://bugs.openjdk.org/browse/JDK-8314754) either, probabaly should get its own ticket. - PR Comment: https://git.openjdk.org/jfx/pull/1476#issuecomment-2183979660
Re: RFR: 8088923: IOOBE when adding duplicate categories to the BarChart [v5]
> This PR provides the test case given in the JBS issue, and a simple fix for > the index calculation when inserting data after previous data with duplicate > categories. > > Also, I've added a comment to `BarChart`s javadoc, clarifying the behavior > that was apparently assumed (but broken) previously. > > The index lookup is skipped for performance reasons if there are no > duplicates, corresponding to the previous implementation. > Further optimizations would be possible, but probably are not really helpful > without more extensive changes. The previous code already loops over all > categories to check if they are present, typically nested in a loop adding > many data items, thus already scaling quadratically when adding lots of > mostly unique data points. Markus Mack has updated the pull request incrementally with one additional commit since the last revision: add javadoc clarification - Changes: - all: https://git.openjdk.org/jfx/pull/1476/files - new: https://git.openjdk.org/jfx/pull/1476/files/6c0ccb80..7a943fcf Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1476&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1476&range=03-04 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1476.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1476/head:pull/1476 PR: https://git.openjdk.org/jfx/pull/1476
Re: RFR: 8088923: IOOBE when adding duplicate categories to the BarChart [v4]
On Fri, 21 Jun 2024 22:07:22 GMT, Andy Goryachev wrote:
>> Markus Mack has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> testAddingDataAtIndex & fix
>
> modules/javafx.controls/src/main/java/javafx/scene/chart/BarChart.java line
> 227:
>
>> 225: var uniqueCategories = new HashSet();
>> 226: for (var entry : seriesCategoryMap.entrySet()) {
>> 227: Series s = entry.getKey();
>
> would it make sense to add a comment explaining the reasoning for this more
> complex code?
I've extended the code comment.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1476#discussion_r1649628640
