On Wed, 12 Mar 2025 00:55:26 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Break up long lines of code
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
> line 193:
>
>> 191: else {
>> 192: CHANGE_LISTENERS.add(this, listener1);
>> 193: }
>
> These can be
>
> switch (listener1) {
> case InvalidationListener il -> INVALIDATION_LISTENERS.add(this, il);
> default -> CHANGE_LISTENERS.add(this, listener1);
> }
>
> but I don't know if it helps.
I may change my mind later when switch expressions have become a bit more
common, but currently I think the if/else version is more readable.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
> line 422:
>
>> 420:
>> 421: private void assertInvalidationListenerIndex(int index) {
>> 422: assert index < invalidationListenersCount : index + " >= " +
>> invalidationListenersCount + ", results would be undefined";
>
> Do we allow `assert` in production code? This will require to run the
> application with `ea`. Or are these intended to be skipped in production and
> are for tests only?
They're useful during testing (when `-ea` is passed, which it generally is).
They will help catch bugs if this code is ever refactored. As for the current
implementation, I'm confident that it will never trip these asserts. It also
serves as a bit of documentation. I'm fine with removing these -- I placed
them in separate methods to not mess up inlining of compiled code (asserts will
count against the method length limit if placed directly in the method). As it
is now, they will be completely eliminated when not running with `-ea`.
As far as allowing goes -- FX is full of asserts already, so I guess its
allowed?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990396334
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990396289
