Hi Tagir,
nice work. Only a partwise review for TreeMap.java, did not yet look at the
tests.
remapValue:
711 } else {
712 // replace old mapping
713 t.value = newValue;
714 return newValue;
715 }
Should we increase the modification count here? AFAICS
Map::computeIfPresent(), Map::compute(), Map::merge() use put() to replace
existing value which would have increased TreeMap::modCount .
Same for mergeValue().
--
At various places, the "@throws ConcurrentModificationException" javadoc:
619 * @throws ConcurrentModificationException if it is detected that
the
620 * mapping function modified this map
is missing the period.
---
We could exchange all the various
if (key == null)
throw new NullPointerException();
lines on comparator==null paths with
Objects.requireNonNull(key).
---
Style nits & bikeshedding:
I would place both versions of getNewValueAndCheckModification() together
with the other new low level utility functions (addEntry,
addEntryToEmptyMap). I may also have named them somewhat differently, maybe
"callMappingFunctionWithCheck" resp. "callRemappingFunctionWithCheck".
---
private void addEntry(K key, V value, int cmp, Entry<K, V> parent) {
For clarity, could we make cmp a boolean "leftOrRight" or are you afraid
that would cost too much performance?
---
About the CSR: I guess this has already been discussed but I feel that
changing the checking behavior of computeIfPresent() and friends to now
detect ConcurrentModificationException has nothing really to do with this
change and could be left out.
Thank you,
On Tue, Mar 24, 2020 at 3:41 AM Tagir Valeev <amae...@gmail.com> wrote:
> Hello!
>
> A gentle reminder to review the specialized implementation of TreeMap
> methods. The latest webrev is here:
> http://cr.openjdk.java.net/~tvaleev/webrev/8176894/r4/
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8176894
>
> CSR is already approved (thanks to Joe Darcy and Stuart Marks):
> https://bugs.openjdk.java.net/browse/JDK-8227666
> So only code review is necessary. Also, no sponsorship is necessary.
>
> Here's the previous discussion
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-October/062859.html
> Here's benchmarking results
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-February/064901.html
>
> Let's push it into Java 15!
>
> With best regards,
> Tagir Valeev.
>
