search

vigna / fastutil

fastutil extends the Java™ Collections Framework by providing type-specific maps, sets, lists and queues.
Apache License 2.0
1.76k stars 196 forks source link

Fix AbstractMap containing a stack overflow containsValue #234

Closed techsy730 closed 3 years ago

techsy730 commented 3 years ago

If both the default containsValue and the default values methods are inherited by an implementating class, they will get a StackOverflowError on trying to test for a value being present.

This is because the default values().contains calls containsValue, but the default containsValue calls values().contains.

Now make the default containsValues loop over the entry set.

Add a comment that you should probably override this is your values() has a better implementation. Same also retroactively applied to containsKey and keySet().

Finally, retrofits AbstractInt2IntMapTest to extendInt2IntMapGenericTest, with a dummy testonly implementation that implements the bare minimum for a mutable Int2Int map. This is to test as many of the AbstractInt2IntMap's default methods as possible. This gives it the set of tests we apply to all other Int2IntMap impls.

techsy730 commented 3 years ago

Hopefully not too late for 8.5.2.

If it is, I don't think it is a big enough issue for a 8.5.3 alone. It can just be rolled up into whatever other fixes do deserve it.

vigna commented 3 years ago

No, we're definitely in time for 8.5.2.

techsy730 commented 3 years ago

Ok, I am going to leave this change as is most likely unless you spot something major.

vigna commented 3 years ago

OK, I merged in and removed my commit about varargs (it's a mess—you get confusion between the constructor with a size argument and the constructor with one element). I had to do some github black sorcery—I just hope I didn't delete any of your merges.