On Thu, 24 Aug 2023 22:30:04 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> javadoc, dispose > > modules/javafx.swing/src/main/java/javafx/embed/swing/SwingNode.java line 377: > >> 375: Disposer.removeRecord(disposerRecRef); >> 376: disposerRecRef = null; >> 377: } > > I think this should be part of the first `if`, or an `if` by itself, and not > here. `lwFrame`, `rec` and `disposerRecRef` will all either be `null` or be > set to some value. The only case this may not happen is that after `lwFrame` > is set, an exception occurs. You could make it very safe, or just assume > that when `lwFrame` is not `null` that all clean up actions should be taken > -- if an exception did occur during set-up, then it's also fine that one can > occur during clean-up, no need to make it more robust for situations that > can't happen. > > There really is no need to start second guessing yourself when the logic is > solid. Either all those variables are non-`null` and checking one of them is > sufficient, or they are all `null` and checking each in turn is pointless (it > is just confusing because programmers may assume now that is a situation that > can actually occur). > > Please revert to the old code. @hjohn I guess i have updated as per suggestion..Let me know if this is ok as per you..if yes, please approve.. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1219#discussion_r1306859824
