Asking about the interesting behaviours of TreeMap.putAll

[David Holmes]

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);
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>