On Sun, 9 Jul 2023 19:12:24 GMT, Jose Pereda <[email protected]> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add newline at end of ConditionalBinding file
>
> modules/javafx.base/src/main/java/javafx/beans/Subscription.java line 77:
>
>> 75: */
>> 76: default Subscription and(Subscription other) {
>> 77: Objects.requireNonNull(other, "other cannot be null");
>
> As per javadoc, this equivalent to `Subscription.combine(this, other)`, so
> then, couldn't you just call it instead of doing a different implementation?
I didn't quite see your point immediately, I thought you meant we don't need
this method, but you meant why not do:
default Subscription and(Subscription other) { return
Subscription.combine(this, other); }
I agree this would work, and I don't mind it either way, but calling `combine`
does incur a bit more overhead (when called with only a list of 2). The reason
I think `and` is a bit more efficient this way is that I think two variable
captures vs a `List` with 2 elements will be slightly more efficient. Not that
I thought about it that long when implementing it, I just wanted to avoid
creating the varargs array, and then the `List` if it isn't strictly needed.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257543221
