On Tue, 1 Sep 2020 00:23:38 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> While adding unit tests, I noticed that I missed an important edge-case that >> has to be considered when computing if a >> list was modified. The initial implementation assumed that >>> the list was modified if >>>1. it was not empty (modified by calling `#clear()`), or if >>>2. it was modified as result of the `#addAll()` call. >> >> However, a non-empty list is not modified either if `setAll` is called with >> an equal list. The PR has been updated to >> take this into account and unit tests have been added. Note that the current >> implementation is rather complex and could >> be greatly simplified by making a copy of the list before modification. .i.e >> List<E> prev = List.copyOf(this); >> clear(); >> addAll(col); >> return !equals(prev); >> >> Please let me know which solution you prefer. > > One overall comment while we are waiting for your OCA to be approved. > > I don't think the complexity of this proposed fix to `setAll` is warranted. I > would prefer a simpler fix that returns > `false` if both the current list and the new list are empty, and `true` > otherwise. This method is essentially a > convenience method that does: List::clear > List::addAll(...) > > Given this, it seems reasonable for `setAll` to return true if either `clear` > or `addAll` would modify the list. > > If there is a good justification for handling the corner case of calling > `setAll` with the same list of elements in > exactly the same order (and I am not sure that there is), then a better > approach might be to do the check before > actually modifying the list, returning early if the new list and the current > list were identical. In that case, the > list really would be unmodified and no listeners would be notified. Unfortunately, I don't see @TheMrMilchmann OCA in the approval queue. Could you please (re)send it to oracle-ca...@oracle.com? Thanks! ------------- PR: https://git.openjdk.java.net/jfx/pull/284
