On Wed, 12 Mar 2025 01:40:02 GMT, John Hendrikx <[email protected]> wrote:
>> 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?
I don't mind personally, I just know some people "frown upon" `assert` in
production code. @kevinrushforth can comment on it, but I don't foresee a
problem.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990409315
