* Thomas Stüfe:

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?

It's not a structural modification, is it? Only structural modifications increase the modification count, as far as I can see.

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.

On Sun 29. Mar 2020 at 18:48, Martin Buchholz <martinrb@google.com> wrote:

I'm fine with the latest version

On Sun, Mar 29, 2020 at 1:29 AM Tagir Valeev <amaembo@gmail.com> wrote:

...

...

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.

Sigh ... my own consistent practice has been to *not* put periods at the end of @throws clauses, but obviously other maintainers ended up elsewhere.

Oops sorry. If it is important for you, I can live without the period...

Ok, I will remove the periods. They are also absent in the corresponding methods in HashMap. On Mon, Mar 30, 2020 at 4:33 AM Roger Riggs <Roger.Riggs@oracle.com> wrote:

Hi,

Usually the @throws, @param, @return, etc clauses are not complete sentences. Only complete sentences need a period. And most of OpenJDK omits them.

$.02, Roger

On 3/29/20 2:22 PM, Thomas Stüfe wrote:

...

On Sun 29. Mar 2020 at 18:48, Martin Buchholz <martinrb@google.com> wrote:

...

I'm fine with the latest version

On Sun, Mar 29, 2020 at 1:29 AM Tagir Valeev <amaembo@gmail.com> wrote:

...

...

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.

Sigh ... my own consistent practice has been to *not* put periods at the end of @throws clauses, but obviously other maintainers ended up elsewhere.

Oops sorry. If it is important for you, I can live without the period...