On Wed, 12 Mar 2025 01:52:16 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> 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. I think I left these in because of the need to subclass these, and the subclasses are expected to work in a certain way and must be highly optimized. Normally, I'd just make them hard checks (with `IllegalStateException` or `IllegalArgumentException`), but for optimal performance it is better to do it with asserts in this case (I don't want it to be too much slower than `ExpressionHelper`). With the asserts, if you write any kind of test code for your subclasses, then it will fail quickly if you made a mistake. As said, I'm confident these subclasses that we have now are implemented correctly, but who knows, in the future we may want to use similar code for ListChangeListeners or something... ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990423826
