Fix/phases etc #97
Fix/phases etc #97
Conversation
|
Review by @DarkDimius @gzm0 |
|
That's all |
| @@ -329,8 +329,8 @@ object Contexts { | |||
|
|
|||
| def withProperty(prop: (String, Any)): this.type = withMoreProperties(moreProperties + prop) | |||
|
|
|||
| override def withPhase(pid: PhaseId): this.type = withPeriod(Period(runId, pid)) | |||
| override def withPhase(phase: Phase): this.type = withPhase(phase.id) | |||
| def withNewPhase(pid: PhaseId): this.type = withPeriod(Period(runId, pid)) | |||
There was a problem hiding this comment.
I would prefer to have all methods in this class start with "set" instead of "with"(as in #98). It will clearly indicate that there will be mutation.
There was a problem hiding this comment.
Agreed. Let's do this, and at the same time rename all withNewXYZ methods in FreshContext to WithXYZ.
|
I think we all agree on the finishing touches and that we want to combine with the with...->set... renamings. Who wants to take the lead on this? |
|
I've already done them in #98 , I'll make them rebased on this branch once again. |
|
@DarkDimius OK. Can you also shepard this PR? Thanks! |
|
Ok, I will. |
|
I don't see why putting the run test inside IMHO doing so is a small cost for the simplification of the contract ("is this valid" v.s. "is this valid, given we are not in the run this denotation originated from"). |
Aims 1) next/prev should be context-independent. Phase now stores its ContextBase in a field. 2) More robust handling of phaseNamed and the phase properties erasedTypes, flattened, refchecked, etc. These were previously dependent on when the first call to any of these methods was made, which led to a data race. There is now an init method in phases which centralizes all necessary intialization. It is checked that a phase is initialized only once.
Aim: Avoid expensive scanning of definitions in denotations other than the initial one. Technique: 1. If phase is erased, type params is Nil (except for Array class) 2. Otherwise, type params is the same as it was in initial phase.
Previous version was wrong because every context is a fresh context at runtime, so the overriding version would always be executed, which means that withPhase becomes a side-effecting operation!
The logic for dealing with periods in denotation histories did not correctly take into account the case where the current validity period of a denotation ends some phases before the next type transformer starts. And there was an off-by-one error in startPid. And Types#computeDenot erroneously tried to reload denotations even the run did not change.
And avoid name clashes
|
LGTM |
|
merged as part of #100 |
This contains the core fixes for phases and denotation transformers to unblock Dmitry and Tobias