On Fri, 22 Jan 2021 17:48:15 GMT, Kevin Rushforth <[email protected]> wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review correction
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
> line 582:
>
>> 580: try {
>> 581: TreeItem rootItem = table.getRoot();
>> 582: if (rootItem == null |
>> rootItem.getChildren().isEmpty()) return false;
>
> That should be `||` (boolean OR). In addition to being less clear, this will
> get an NPE if `rootItem` is ever null, since the bit-wise `|` operator
> doesn't short-curcuit the rest of the `if` test if the first term is true.
>
> Given that this would have caused a regression, and that we don't have a test
> that would catch it, can you add a test that sets the root to null and calls
> sort? It will fail with this version of the fix, but would pass without the
> fix and should pass once you update your fix.
Sorry about that, It was a typo. But it did reveal the missing test.
Corrected the operator to `||` and added a test as you suggested which fails
only with the commit 1 of this PR.
-------------
PR: https://git.openjdk.java.net/jfx/pull/384
