JDK 10 RFR of 8176894: Provide specialized implementation for default methods putIfAbsent, computeIfAbsent, computeIfPresent, compute in TreeMap

[Claes Redestad]

Hi Sergey,

this looks good to me*, but I can't help wonder if the modCount checking 
is something that should be done separately as a bug fix (with a higher 
priority) and be backported to 8 and 9? Alternatively re-categorize this 
fix as such.

Thanks!

/Claes

* I wouldn't mind seeing the cleanup Martin suggested, i.e., write the 
remapValue as:

     private V remapValue(Entry<K, V> t, K key, BiFunction<? super K, ? 
super V, ? extends V> remappingFunction) {
         V newValue = remappingFunction.apply(key, t.value);
         if (newValue == null) {
             deleteEntry(t);
         } else {
             // replace old mapping
             t.value = newValue;
         }
         return newValue;
      }

On 2017-03-28 21:22, Sergey Kuksenko wrote:
> Friendly reminder.
>
> Have you have chance to review the latest version?
>
>
> On 03/17/2017 12:45 PM, Sergey Kuksenko wrote:
>> Hi, All.
>>
>> I realized (thanks Tagir V.) that if we don't check modCount after 
>> calling mapping function we may get corrupted tree structure.
>> new webrev for review:
>> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.01/
>>
>> On 03/17/2017 11:29 AM, Martin Buchholz wrote:
>>> Thanks!  This looks pretty good.  I have a similar effort in 
>>> progress to improve bulk collection operations, most of which made 
>>> it into jdk9.
>>>
>>> ---
>>>
>>> Please use standard java.util whitespace, as Aleksey suggested.
>>>
>>> ---
>>>
>>> Below (and in compute) I wpuld simply
>>> return newValue;
>>> saving a line of code and making it clearer that we are returning 
>>> the result of the remappingFunction
>>>
>>>  676     private V remapValue(Entry<K, V> t, K key, BiFunction<? 
>>> super K, ? super V, ? extends V> remappingFunction) {
>>>  677         V newValue = remappingFunction.apply(key, t.value);
>>>  678         if (newValue == null) {
>>>  679             deleteEntry(t);
>>>  680             return null;
>>>  681         } else {
>>>  682             // replace old mapping
>>>  683             t.value = newValue;
>>>  684             return newValue;
>>>  685         }
>>>  686     }
>>> ---
>>>
>>> This code is surely tested but testing could also surely be 
>>> improved.  That's probably not your job though (it may be mine!)
>>>
>>> I would probably try hand-injecting some bugs into a copy of the 
>>> code and seeing if our jtreg tests catch it, to increase coverage 
>>> confidence.
>>>
>>>
>>> On Thu, Mar 16, 2017 at 12:04 PM, Sergey Kuksenko 
>>> <sergey.kuksenko at oracle.com <mailto:sergey.kuksenko at oracle.com>> wrote:
>>>
>>>     Hi All,
>>>
>>>     Please, review:
>>>     https://bugs.openjdk.java.net/browse/JDK-8176894
>>>     <https://bugs.openjdk.java.net/browse/JDK-8176894>
>>> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/
>>> <http://cr.openjdk.java.net/%7Eskuksenko/corelibs/utils/8176894/webrev.00/> 
>>>
>>>
>>>     The issue was created for JDK10 in order to don't disturb JDK9
>>>     before launch.
>>>
>>>     --     Best regards,
>>>     Sergey Kuksenko
>>>
>>>
>>
>