Copilot commented on code in PR #705:
URL:
https://github.com/apache/commons-collections/pull/705#discussion_r3555245849
##########
src/test/java/org/apache/commons/collections4/multiset/HashMultiSetTest.java:
##########
@@ -57,6 +58,17 @@ void testDeserializeRejectsNonPositiveCount() throws
Exception {
}
}
+ @Test
+ void testAddClampsCountAndSizeToIntegerMaxValue() {
+ final HashMultiSet<String> set = new HashMultiSet<>();
+ set.add("X", Integer.MAX_VALUE);
+ set.add("X", 1);
+ assertEquals(Integer.MAX_VALUE, set.getCount("X"));
+ assertEquals(Integer.MAX_VALUE, set.size());
+ set.add("Y", Integer.MAX_VALUE);
+ assertEquals(Integer.MAX_VALUE, set.size());
+ }
Review Comment:
This test asserts `size()` clamps on additional adds, but it doesn’t
validate the `Collection.size()` contract when removing elements from a
multiset whose true cardinality remains > `Integer.MAX_VALUE`. An
implementation that clamps the stored `size` field (instead of clamping on
return) will typically fail this scenario.
##########
src/main/java/org/apache/commons/collections4/bag/AbstractMapBag.java:
##########
@@ -206,12 +206,12 @@ public boolean add(final E object, final int nCopies) {
modCount++;
if (nCopies > 0) {
final MutableInteger mut = map.get(object);
- size += nCopies;
+ size = (int) Math.min((long) size + nCopies, Integer.MAX_VALUE);
Review Comment:
Clamping the stored `size` field to `Integer.MAX_VALUE` during `add()` makes
`size()` diverge from the sum of per-element counts once multiple elements are
added (e.g., adding two keys with `Integer.MAX_VALUE - 1` each). Subsequent
`remove(...)` / iterator-remove paths decrement `size` (see `size -= ...` and
`size--` elsewhere in this class), which can drop `size()` below
`Integer.MAX_VALUE` even while the true cardinality is still >
`Integer.MAX_VALUE`, violating the `Collection.size()` contract and potentially
producing incorrect/negative sizes.
##########
src/main/java/org/apache/commons/collections4/multiset/AbstractMapMultiSet.java:
##########
@@ -313,11 +313,11 @@ public int add(final E object, final int occurrences) {
if (occurrences > 0) {
modCount++;
- size += occurrences;
+ size = (int) Math.min((long) size + occurrences,
Integer.MAX_VALUE);
Review Comment:
Clamping the stored `size` field here loses information once the multiset’s
true cardinality exceeds `Integer.MAX_VALUE` across multiple distinct elements.
Since `remove(...)` and iterator removals later do `size -= occurrences` /
`size -= count`, `size()` will drop below `Integer.MAX_VALUE` even when the
multiset still contains > `Integer.MAX_VALUE` total elements, which contradicts
the `Collection.size()` contract and can make `size` inconsistent with the
backing map’s counts.
##########
src/test/java/org/apache/commons/collections4/bag/HashBagTest.java:
##########
@@ -57,6 +58,18 @@ void testDeserializeRejectsNonPositiveCount() throws
Exception {
}
}
+ @Test
+ void testAddClampsCountAndSizeToIntegerMaxValue() {
+ final HashBag<String> bag = new HashBag<>();
+ bag.add("X", Integer.MAX_VALUE - 1);
+ bag.add("Y", Integer.MAX_VALUE - 1);
+ assertEquals(Integer.MAX_VALUE, bag.size());
+ bag.add("X", 10);
+ assertEquals(Integer.MAX_VALUE, bag.getCount("X"));
+ assertEquals(Integer.MAX_VALUE, bag.size());
+ assertEquals(2, bag.uniqueSet().size());
+ }
Review Comment:
This test covers clamping on additional adds, but it doesn’t exercise the
required `Collection.size()` behavior after removals when the true cardinality
is still > `Integer.MAX_VALUE` (e.g., after adding two elements with very large
counts). Without that, an implementation that clamps the stored `size` field
can still be incorrect under `remove(...)`/iterator removal while this test
passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
