Incorrect handling of size in all primitive maps
When adjusting elements in a primitive hashmap, no check is made whether the new value equals the no_entry_value. If this is the case. The entry should be removed and the size should be updated accordingly.
For example, take a simple TIntIntHashMap. Call put(1,1) and then call adjust(1,-1). The result is a map of size 1, while the value for key 1 is 0, which equals the no_entry_value. Hence, the size should be 0.
This applies also to the adjustOrPut method, the transformValues method, the forEach... methods etc. Basically, everywhere where the values array is updated, a check should be done if the result equals the no_entry_value and if so, the states array should be set to FREE and the size should be updated.
Comments (3)
-
reporter -
reporter More comments from original bug...
Rob Eden
After looking at this, I think I disagree with the bug. Here's my test case: :::java public void testAdjustToNoEntry() { TObjectIntMap<String> map = new TObjectIntHashMap<String>();
assertEquals( 0, map.getNoEntryValue() ); assertEquals( 0, map.get( "NotInThere" ) ); map.put( "Value", 1 ); assertEquals( 1, map.size() ); assertEquals( 1, map.get( "Value" ) ); assertTrue( map.containsKey( "Value" ) ); assertTrue( map.containsValue( 1 ) ); assertTrue( Arrays.equals( new int[] { 1 }, map.values() ) ); map.adjustValue( "Value", -1 ); assertEquals( 1, map.size() ); assertEquals( 0, map.get( "Value" ) ); assertTrue( map.containsKey( "Value" ) ); assertTrue( map.containsValue( 0 ) ); assertTrue( Arrays.equals( new int[] { 0 }, map.values() ) ); }
So, we have a result where the value stored is actually the no_entry_value, or zero. That's returned from the get() and everything appears to function correctly. Obviously, at this point the only way to tell that the value is really in the map is to use containsKey().
However, this behaves exactly like the java.util collections when null is put in a map for a value. Likewise, null would be returned from a get() so you'd only be able to tell from contains().
So, I believe this is functioning as it should. If you set a value to be the same as the marker for no value (whatever it is), then yes, things get a bit more confusing. But, I believe they're operating correctly.
I'm open to rebuttal though...
Anonymous
I agree that, if you interpret the no_entry_value to correspond to null, then you are right, i.e. the size is handled correctly and should not be updated. This behavior is indeed consistent with the java collections framework. Also, in that case, the behavior of adjustValue is correct, since it only changes the stored value. However, if that is the case, then the implementation of equality is wrong, see bug [3432399] as shown by the following code:
// initialize the int maps TIntIntMap intMap1 = new TIntIntHashMap(); TIntIntMap intMap2 = new TIntIntHashMap(); intMap1.put(1, 1); intMap1.put(2, 2); intMap2.put(1, 1); intMap2.put(2, 2); // confirm symmetric equality assert intMap1.equals(intMap2); assert intMap2.equals(intMap1); // in the second map, change value for 2 to 0 (null) intMap2.put(2, 0); // confirm symmetric equality assert !intMap2.equals(intMap1); assert !intMap1.equals(intMap2); // ERROR here
In my view, allowing for null-elements is an important design decision. Depending on the use of the maps, you may have different requirements. For example, when using a map to represent a multiset, or bag, of integers, then 0 and no_entry_value should be equal.
-
reporter - changed status to invalid
I've added the test case TPrimitivePrimitiveHashMapTest.testIssue3 which runs the test described in the last comment. The comparison does not fail in the manner described, so I believe this issue was resolved.
- Log in to comment
Original SF bug
Comments...
Johan Parent:
Johan Parent:
Anonymous
Anonymous
Rob Eden:
Anonymous:
Anonymous:
Rob Eden:
Anonymous:
Rob Eden