Removing useless GraphOccurrence.Global & some encapsulation
#11419
Conversation
JaroslavTulach
added
CI: Ready to merge
JaroslavTulach
requested review from
4e6,
hubertp and
Akirathan
as code owners
JaroslavTulach
added
the
CI: Clean build required
JaroslavTulach
changed the title
GraphOccurrence.GlobalGraphOccurrence.Global & encapsulation
JaroslavTulach
changed the title
GraphOccurrence.Global & encapsulationGraphOccurrence.Global & some encapsulation
|
Jaroslav Tulach reports a new STANDUP for yesterday (2024-10-27): Progress: .
Next Day: Bank holiday. Establishment of Czechoslovakia. |
| @@ -25,9 +25,15 @@ case class InlineContext( | |||
| freshNameSupply: Option[FreshNameSupply] = None, | |||
| passConfiguration: Option[PassConfiguration] = None, | |||
| pkgRepo: Option[PackageRepository] = None | |||
| ) { | |||
| ) extends AutoCloseable { | |||
There was a problem hiding this comment.
Isn't it easier to make the InlineContext closable than wrap it into InlineContextResource?
| @@ -143,7 +143,6 @@ protected Graph readObject(Input in) throws IOException { | |||
|
|
|||
| var nextIdCounter = in.readInt(); | |||
| var g = new Graph(rootScope, nextIdCounter, links); | |||
| g.freeze(); | |||
There was a problem hiding this comment.
Why remove g.freeze() from here? Is it because of failing tests? Is it not needed anymore?
There was a problem hiding this comment.
Is it because of failing tests?
No, it is not because of a test. Rather:
- Avoid using
assertin Scala, useassertInJvmto elide it in production #11027 - assert in AliasAnalysis when
runEngineDistribution --debug#11022
Is it not needed anymore?
freezeing doesn't work with DebugEvalNode right now.
| val nextId = _nextIdCounter | ||
| if (nextId < 0) { | ||
| throw new IllegalStateException("Cannot emit new IDs. Frozen!") |
There was a problem hiding this comment.
IMHO it is better to have the explicit boolean frozen field, than encoding the frozen state as nextId == -1.
There was a problem hiding this comment.
I guess, my original idea was to not serialize nextIdCounter in serde, but initialize it to -1 after deserialization - e.g. freeze it. But (as freezing doesn't work - #11022) I had to give up on that idea. Now the code is ready for freezing (by assigning the counter to -1), but there is no freezing happening. Hopefully it will, one day.
Pull Request Description
Work on #11365 made me realize that
GraphOccurrence.Globalisn't really used anywhere. Simplifying the code by removing it and associated methods.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,