Asking about the interesting behaviours of TreeMap.putAll (original) (raw)

David Holmes david.holmes at oracle.com
Mon Mar 5 10:53:31 UTC 2012


Charles,

I just realized that your webrev is for Map not AbstractMap and that it is Map the states putAll does a put() on each entry. This changes things. It is much harder, perhaps not even possible to change Map even if we think the spec is overly constraining.

David

On 5/03/2012 8:09 PM, David Holmes wrote:

FYI CR 7151065 filed.

David On 5/03/2012 7:50 PM, David Holmes wrote: On 5/03/2012 7:13 PM, Charles Lee wrote:

Hi David,

I am sorry for the unclear. I was suggesting to add some implementation notes on the TreeMap.... The change as you suggested is great. The patch is @ http://cr.openjdk.java.net/~littlee/map-putall/webrev.00/ <http://cr.openjdk.java.net/%7Elittlee/map-putall/webrev.00/> . Is it ok for you? Its okay with me in principle but as this is a spec change it has to go through the internal CCC process. And first it needs a bug filed. And ideally our collections experts (internal and external) will chime in. David ----- On 03/05/2012 12:11 PM, David Holmes wrote: Hi Charles,

I'm not quite sure what you are suggesting. In my opinion all that is needed is for AbstractMap.putAll to read: Copies all of the mappings from the specified map to this map (optional operation). The behavior of this operation is undefined if the specified map is modified while the operation is in progress. This implementation iterates over the specified map's entrySet() collection, and calls this map's put operation once for each entry returned by the iteration. --- If AbstractMap.putAll does not allow for a non-put() based implementation then TreeMap is in violation of the spec. In which case TreeMap should not extend AbstractMap. David ----- On 5/03/2012 1:42 PM, Charles Lee wrote: Hi David,

I also notice that in the AbstractMap doc, it also says: "The documentation for each non-abstract method in this class describes its implementation in detail. Each of these methods may be overridden if the map being implemented admits a more efficient implementation. " If this is the logic[1], shall we add some implementation notes in the subclass of AbstractMap when some default behaviours have been changed? From the spec, the only implementation of putAll I can find from the TreeMap is using the put method. [1] The logic means: a. We have to place the implementation note in every specified method api b. The subclass feels free to change the implementation. On 03/02/2012 05:02 PM, David Holmes wrote: HI Charles,

I tend to agree with you. In this case, in my opinion, AbstractMap.putAll has no business saying that it is equivalent to calling put() as that should be part of the implementation note, not the actual spec. Subclasses should be free to implement putAll in the most efficient manner possible as TreeMap does. David On 2/03/2012 6:17 PM, Charles Lee wrote: Hi guys,

I have a small test case[1] and the two invokes of putAll have different behaviors: the first invocation does not use the override put but the second invocation does. The root cause about this can be find in the TreeMap code: /if (size==0 && mapSize!=0 && map instanceof SortedMap) { Comparator c = ((SortedMap)map).comparator(); if (c == comparator || (c != null && c.equals(comparator))) { ++modCount; try { buildFromSorted(mapSize, map.entrySet().iterator(), null, null); } catch (java.io.IOException cannotHappen) { } catch (ClassNotFoundException cannotHappen) { } return; } }/ When meet some situations, buildFromSorted will be invoked instead of put. I understand it is a speed up, but it may confuse people: "I need my own put because of something, but interestingly sometimes it will not be called when putAll and I do not find the reason from the api spec." From the api spec of TreeMap's putAll, it says nothing about put. But from the api spec of AbstractMap's putAll and Map's putAll, they said: / "The effect of this call is equivalent to that of calling put(k, v) on this map once for each mapping from key k to value v in the specified map. " /The spec clearly say that, putAll will use put, that means, we can not use a putAll in an override put. Otherwise, it will recursive endlessly. So can I use a putAll in the override put method in an class which extends the TreeMap? [1] public class TreeMapTest<K, V> extends TreeMap<K, V> { @Override public V put(K key, V value) { System.out.println(key + " : " + value); return super.put(key, value); } /** * @param args */ public static void main(String[] args) { TreeMapTest<Integer, Integer> mTreeMap = new TreeMapTest<>(); TreeMap<Integer, Integer> mt = new TreeMap<>(); mt.put(1, 1); mTreeMap.putAll(mt); mTreeMap.clear(); mTreeMap.put(2, 2); mTreeMap.putAll(mt); } }



More information about the core-libs-dev mailing list