Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Thu, 28 Mar 2024 06:07:24 GMT, Karthik P K wrote: >> You don't need to return a list, you create it ahead of time like was done >> in line 167 >> >> List indices = new ArrayList<>(); >> >> and the add the elements in `forEach`. > >> Why do the double-iteration pattern here and not do the `peek` operation in >> a `forEach` like in the other 2 places? > > Yes, if the usage of `forEach` in previous 2 places are correct then I would > like to see similar change here as well. I changed this to the pre-allocated pattern like in the other places. An initial capacity of `indices.length + 1` optimizes the worst case (all given indices need to be set) which probably occurs quite often in real-world usage. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542535713
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 23:21:34 GMT, Nir Lisker wrote: >> `forEach` is void, so we can not return a list afterwards. > > You don't need to return a list, you create it ahead of time like was done in > line 167 > > List indices = new ArrayList<>(); > > and the add the elements in `forEach`. > Why do the double-iteration pattern here and not do the `peek` operation in a > `forEach` like in the other 2 places? Yes, if the usage of `forEach` in previous 2 places are correct then I would like to see similar change here as well. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542358972
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 23:24:51 GMT, Marius Hanl wrote: >>> In the java.util.stream package >>> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects) >>> it is mentioned that `forEach()` method operates only via side-effects. So >>> do you think we should avoid using `forEach()` here and iterate the >>> generated list separately to clear selected index? >> >> `forEach` is used correctly here. From the docs: >>> With the exception of terminal operations >>> [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer)) >>> and >>> [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)), >>> side-effects of behavioral parameters may not always be executed when the >>> stream implementation can optimize away the execution of behavioral >>> parameters without affecting the result of the computation. >> >>> Another idea is to use `toList()`, which is a very efficient operation and >>> then iterate over it. >> >> That's still 2 iterations. If the code is not performance-critical it >> doesn't matter. > >> That's still 2 iterations. > > Yes, but one advantage here: > We currently do `final List removed = new > ArrayList<>(c.getRemovedSize());`, > where we allocate a list with a size, that is probably too big since we > filter the removed items. > So with `toList`, we at least get back a list with the correct size. > But true, that we technically iterate twice then. It probably does not matter > too much. > `forEach` is used correctly here. From the docs: > > > With the exception of terminal operations > > [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer)) > > and > > [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)), > > side-effects of behavioral parameters may not always be executed when the > > stream implementation can optimize away the execution of behavioral > > parameters without affecting the result of the computation. > Thanks for pointing this out. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542359758
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 13:41:21 GMT, Nir Lisker wrote: > That's still 2 iterations. Yes, but one advantage here: We currently do `final List removed = new ArrayList<>(c.getRemovedSize());`, where we allocate a list with a size, that is probably too big since we filter the removed items. So with `toList`, we at least get back a list with the correct size. But true, that we technically iterate twice then. It probably does not matter too much. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542147474
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 23:18:43 GMT, Marius Hanl wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java >> line 773: >> >>> 771: .collect(Collectors.toList()); >>> 772: >>> 773: sortedNewIndices.forEach(this::set); >> >> Why do the double-iteration pattern here and not do the `peek` operation in >> a `forEach` like in the other 2 places? > > `forEach` is void, so we can not return a list afterwards. You don't need to return a list, you create it ahead of time like was done in line 167 List indices = new ArrayList<>(); and the add the elements in `forEach`. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542145635
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 13:51:46 GMT, Nir Lisker wrote: >> drmarmac has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove outdated comment > > modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java > line 773: > >> 771: .collect(Collectors.toList()); >> 772: >> 773: sortedNewIndices.forEach(this::set); > > Why do the double-iteration pattern here and not do the `peek` operation in a > `forEach` like in the other 2 places? `forEach` is void, so we can not return a list afterwards. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542143935
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 09:11:56 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with one additional > commit since the last revision: > > Remove outdated comment modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java line 773: > 771: .collect(Collectors.toList()); > 772: > 773: sortedNewIndices.forEach(this::set); Why do the double-iteration pattern here and not do the `peek` operation in a `forEach` like in the other 2 places? - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541157874
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 12:23:47 GMT, Marius Hanl wrote: >> I'd say .forEach() is used correctly here, according to docs, it guarantees >> execution of the side-effects (add to removed list & clear index), just not >> in any particular order. This way we avoid multiple iteration. > > Another idea is to use `toList()`, which is a very efficient operation and > then iterate over it. > In the java.util.stream package > [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects) > it is mentioned that `forEach()` method operates only via side-effects. So > do you think we should avoid using `forEach()` here and iterate the generated > list separately to clear selected index? `forEach` is used correctly here. From the docs: > With the exception of terminal operations > [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer)) > and > [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)), > side-effects of behavioral parameters may not always be executed when the > stream implementation can optimize away the execution of behavioral > parameters without affecting the result of the computation. > Another idea is to use `toList()`, which is a very efficient operation and > then iterate over it. That's still 2 iterations. If the code is not performance-critical it doesn't matter. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541140317
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 09:07:15 GMT, drmarmac wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
>> line 176:
>>
>>> 174: .distinct()
>>> 175: .filter(removeRowFilter)
>>> 176: .forEach(row -> {
>>
>> In the java.util.stream package
>> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects)
>> it is mentioned that `forEach()` method operates only via side-effects. So
>> do you think we should avoid using `forEach()` here and iterate the
>> generated list separately to clear selected index?
>
> I'd say .forEach() is used correctly here, according to docs, it guarantees
> execution of the side-effects (add to removed list & clear index), just not
> in any particular order. This way we avoid multiple iteration.
Another idea is to use `toList()`, which is a very efficient operation and then
iterate over it.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541003650
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Tue, 26 Mar 2024 16:32:53 GMT, Karthik P K wrote:
>> drmarmac has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove outdated comment
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
> line 176:
>
>> 174: .distinct()
>> 175: .filter(removeRowFilter)
>> 176: .forEach(row -> {
>
> In the java.util.stream package
> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects)
> it is mentioned that `forEach()` method operates only via side-effects. So
> do you think we should avoid using `forEach()` here and iterate the generated
> list separately to clear selected index?
I'd say .forEach() is used correctly here, according to docs, it guarantees
execution of the side-effects (add to removed list & clear index), just not in
any particular order. This way we avoid multiple iteration.
> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
> line 185:
>
>> 183: .mapToInt(TablePositionBase::getRow)
>> 184: .distinct()
>> 185: .forEach(row -> {
>
> Similar comment as above. Here if we do not use `forEach()` on streams, we
> can also avoid using array of size one for keeping count as well right?
We'd need to iterate twice as well (select index & count), with forEach it's
just once.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1540712652
PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1540712756
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
> This PR removes potentially incorrect usages of Stream.peek(). > The changed code should be covered by the tests that are already present. drmarmac has updated the pull request incrementally with one additional commit since the last revision: Remove outdated comment - Changes: - all: https://git.openjdk.org/jfx/pull/1430/files - new: https://git.openjdk.org/jfx/pull/1430/files/676ec3cf..1b285b5b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1430&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1430&range=00-01 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1430.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1430/head:pull/1430 PR: https://git.openjdk.org/jfx/pull/1430
