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.kukse...@oracle.com <mailto:sergey.kukse...@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
