fix: Make SortedList#add return value adhere to List contract #6049
Conversation
CoreRasurae
commented
Oct 28, 2024
CoreRasurae
commented
- Fixes [Bug]: spoon.support.util.SortedList.addAll(...) return value is not JLS compliant #6034
- SortedList.addAll(...) should return a boolean value indicating if the Collection was modified is any way as a result of the call
- Previously addAll(...) would return false if only partial changes occurred and would only return true if all elements had been inserted, or if adding an empty list which in not in accordance to the Java JLS/API as documented in https://docs.oracle.com/javase/8/docs/api/?java/util/Collections.html.
There was a problem hiding this comment.
One minor thing, other than that, please make sure your PR title follows our guidelines (https://github.com/INRIA/spoon/blob/master/CONTRIBUTING.md#guidelines-for-pull-requests). Thanks!
| for (E e : c) { | ||
| ret &= add(e); | ||
| ret |= add(e); |
There was a problem hiding this comment.
As we'll always return true this way (because add always returns true), we can probably also just return !c.isEmpty() (assuming c doesn't get modified in between) or just do ret = true, similar to the default implementation in AbstractList. What do you think? (Seems like the Qodana inspection is a bit aggressive here...)
There was a problem hiding this comment.
Qodana is right i didn't notice it was using bit-wise operations instead of logical operations i have fixed that now
There was a problem hiding this comment.
I hope the PR title is okay now
There was a problem hiding this comment.
Your code before was fine, I definitely prefer it over the short-circuit solution now. I mainly suggested the alternatives because Qodana complained, but I still think the inspection is not helpful in such case at all.
I hope the PR title is okay now
Please give it a short description of what is addressed, so we can use it as commit message when merging (e.g. fix: address SortedList#addAll(...) contract violation). Also, please additionally prefix the title with review: given the PR is ready for review.
There was a problem hiding this comment.
i hope that the title is okay now... trying to follow the procedures that indicate that fic and the number of the issue needs to be present too
There was a problem hiding this comment.
Please use one of my suggested variants or your initial version instead of the ||. The title is fine now.
There was a problem hiding this comment.
Okay, i've reverted the change to use bit-wise logic again.
CoreRasurae
changed the title
CoreRasurae
changed the title
…ests (Fixes INRIA#6034) SortedList.addAll(...) should return a boolean value indicating if the Collection was modified is any way as a result of the call
|
I am unable to merge... The base branch restricts merging to authorized users. |
|
@CoreRasurae Qodana still fails. Could you take a look? @SirYwell would merge afterwards. |
SirYwell
changed the title
The Qodana problem is basically a false positive. A short-circuit operation isn't intuitive there at all imho. |
