Thank you for the review! Here's the updated version:
http://cr.openjdk.java.net/~tvaleev/webrev/8176894/r5/
Martin,
> 676 V newValue;
> 677 int mc = modCount;
> 678 newValue = mappingFunction.apply(key);
> I would never use a C style declaration when an initializing declaration
> would work equally well.
Nice catch, thanks! Fixed
> Prevailing whitespace style here is Entry<K,V> rather than Entry<K, V> but
> Intellij is on your side
Fixed in my changes as well. Though there are still eight occurrences
of <K, V> outside of my changes.
Thomas,
> 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 .
Florian is right: it's not a structural modification. E.g.
Entry.setValue() doesn't increase modCount. Also, same operations on
HashMap don't update it as well.
> 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.
Done.
> We could exchange all the various
>
> if (key == null)
> throw new NullPointerException();
>
> lines on comparator==null paths with
>
> Objects.requireNonNull(key).
Done. Also, updated getEntry() similarly (was not changed before).
> 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".
I grouped together all the added private methods and renamed, as per
your suggestion.
> 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?
I don't think this should harm the performance significantly. However,
"leftOrRight" name sounds confusing for boolean parameter (it's not
evident whether 'true' means 'left' or 'right'). So I named the
parameter "addToLeft".
I'd like to push this changeset in 3-4 days unless I see any
objections or new review comments. Thanks again!
With best regards,
Tagir Valeev.
