Asking about the interesting behaviours of TreeMap.putAll

[David Holmes]

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