New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve null policy in wrapped Java Map
s
#10129
Conversation
override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op) | ||
override def getOrElseUpdate(key: K, op: => V): V = | ||
underlying.computeIfAbsent(key, _ => op) match { | ||
case null => super.getOrElseUpdate(key, op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to clarify super[Which]
here (and below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, super[concurrent.Map].getOrElseUpdate
is not allowed, interesting language limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems null
handling of JConcurrentMapWrapper.putIfAbsent
is not aligned with Scala's semantics, if the underlying map contained a binding with value null
, it will return None
. Would it make sense to fix this?
I guess it's not too important as both concurrent map implementations in the JDK don't support null
. Is there a known / commonly used one that does?
LGTM
override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op) | ||
override def getOrElseUpdate(key: K, op: => V): V = | ||
underlying.computeIfAbsent(key, _ => op) match { | ||
case null => super.getOrElseUpdate(key, op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, super[concurrent.Map].getOrElseUpdate
is not allowed, interesting language limitation.
} | ||
try Option(underlying.compute(key, remap)) | ||
catch { | ||
case PutNull => super.updateWith(key)(remappingFunction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the dance is between
- supporting Scala semantics for
updateWith
(can putSome(null)
) - supporting the underlying map's contract (the
ConcurrentHashMap.compute
method evaluates the argument function only once)
and we can't have both, so for Some(null)
the function will re-evaluate. That looks fine to me.
24d883c
to
094d970
Compare
4d1f8c8
to
7abe9b8
Compare
@som-snytt I pushed a commit to remove the |
I think the non-concurrent case was for efficiency. |
When using `compute`, which has "remove" semantics for `null` value, try alternative means when user wants to put a `null`. In particular, if the underlying map wants to throw NPE, the fallback should do so.
7abe9b8
to
b824b84
Compare
Ah, that's a good reason 👍 thanks. |
Map
s
When using
compute
, which has "remove" semantics fornull
value,try alternative means when user wants to put a
null
. In particular,if the underlying map wants to throw NPE, the fallback should do so.
Follow-up to #10027