RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v13]

[Nir Lisker]

On Wed, 12 Mar 2025 01:40:02 GMT, John Hendrikx <jhendrikx at openjdk.org> 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