On Oct 3 2013, at 22:15 , David Holmes wrote: > On 4/10/2013 1:35 PM, Mike Duigou wrote: >> Hello all; >> >> This is a changeset which improves the consistency of several Map.merge >> implementations for handling of null values. > > It isn't at all clear to me what specification you are using to define the > expected behaviour here.
Most of the new Map methods provide only limited support for null values. This is not due to null-hostility but generally a consequence of trying to avoid the additional overhead of disambiguating between value absent and null value. This includes the result of get() and null returns are also used from the functions to provide special behaviour (usually removal). The merge() spec is typical in that it promises to treat null and absent values the same. Some might suggest this is a cunning plot by Doug to discourage use of null values. Perhaps. ;-) The utility of being able to use null values as sentinels, whether from get() or the use function is primary reason for this behaviour. Making the behaviour of the Map defaults and core collections "not horrible" and "not astonishing" in the presence null values has been a fun challenge. :-) It's sometimes necessitated providing alternative defaults for ConcurrentMap. I believe that the defaults in Map and ConcurrentMap could further diverge over time if it turns out to be a problem that the current Map defaults don't look aggressively enough for concurrent modification. > I would have thought that anyone supplying a remapping function needs to be > aware of whether the target map supports nulls or not, and that the remapping > function should then do the right thing if null is encountered. Instead you > are making the decision to bypass the remapping function if you encounter a > null. Bypassing the function is what merge() does when there's no existing values. This behaviour is not specific to my changes. Here's the logic table I used to construct the tests: current value merged put/rem returned ======= ===== ====== ======= ======== absent null n/a remove null absent newval n/a newval newval null null n/a remove null null newval n/a newval newval oldval null null remove null oldval null result result result oldval newval null remove null oldval newval result result result Alternatively, compute() offers the more general behaviour of always calling the remapping function though it does not offer the opportunity to directly provide a "newValue" (capture by the lambda is the alternative). If I had designed the API I would have had only a single method that provided newValue and unconditionally called the function. This is probably would have been naive--I don't know Doug's reasons behind choosing this behaviour for merge() and compute() though I am certainly curious about why it is done the way it is. Mike > > David > >> The existing unit tests hadn't considered several cases where the result of >> the remapper was not the same as the value. I've restructured the merge >> tests to be more thorough and systematic this revealed a couple of problems. >> >> http://cr.openjdk.java.net/~mduigou/JDK-8024688/0/webrev/ >> >> Like several of the previous patches, this one introduces an alternative >> default for ConcurrentMap to work around issues involving null values where >> the handling in the general Map default would be incorrect. >> >> Mike >>
