scala/scala — comment on commit 5fb26c6a889cf1609823338df8783bf880769b3f
File:  line 0
By pavelpavlov:

Hmm, this implementation seems to be broken for me.

1) All the non-compiler-generated subclasses of AbstractPartialFunction
must call missingCase from apply unless its _isDefnedAt always returns true or its orElse is redefined.
Otherwise PFs returned by orElse on them will be broken.
With quick grep on nightly built scala-libray-src.jar & scala-compiler-src.jar I found 8 such broken classes:
- Function.unlift
- PartialFunction.orElse (non-optimized)
- PartialFunction.andThen
- actors.Futures.awaitWith
- actors.Reactor.waitingForNone
- util.control.Exception.pfFromExceptions
- tools.nsc.interpreter.AbstractOrMissingHandler
- tools.nsc.matching.ParalleMatching.scala, lazy val successMatrixFn

A few other comments on the code:

2) // Question: Need to ensure that fallBack is overwritten before any access
// Is the `synchronized` here the right thing to achieve this?
// Is there a cheaper way?

It is enough to declare 'fallBackField' field as volatile.
As you have not yet exposed just cloned object (in orElse) to anyone,
it is safe to write into volatile 'fallBackField' field then let the object escape.
In that case JMM guarantees that anyone will see right value of the field here.
Moreover, to simplify overall scheme, it is better to remove assignment to the field from the accessor
(moving it back to constructor as in previous commit).
With this you can eliminate both synchronized blocks from the code.

3) It seems that 'orElseFast' is not used anywhere.