Conversation
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with |
d887446 to
3f7fd4b
Compare
| /// This library defines the context objects used for serialization | ||
| /// and deserialization of normalized JSON. | ||
| /// | ||
| /// ### Layer Responsibilities |
There was a problem hiding this comment.
I am trying to relate this back to the code. Where would I find the implementation of the syntax layer and semantics layer? Can you leave some breadcrumbs in the doc?
| /// 1. **Constants**: Resolved first. | ||
| /// 2. **Recordings**: Resolved last. References Constants. |
There was a problem hiding this comment.
What constants and recordings is this referring to? The objects below only have constants. Can you leave some breadcrumbs in the doc so future readers know what this is referring to?
|
|
||
| import 'constant.dart'; | ||
|
|
||
| /// The final deserialization state where all pools are resolved. |
There was a problem hiding this comment.
Document: What is a pool in this context? (I don't think we've used that term before in record_use?)
|
|
||
| /// The final serialization state where all index maps are available. | ||
| final class SerializationContext { | ||
| final Map<Constant, int> constants; |
There was a problem hiding this comment.
Document this. What does the int represent?
|
|
||
| factory Recordings._fromSyntax(RecordedUsesSyntax syntax) { | ||
| final constants = <Constant>[]; | ||
| final context = DeserializationContext(constants: constants); |
There was a problem hiding this comment.
Upon first reading through this PR I was confused that nothing appears to ever be added to this context. That made me wonder: Is there ever a reason to construct a DeserializationContext with a non-empty constants list? If not, maybe DeserializationContext.constants should just be internally initialized to an empty lists to remove the constructor argument and this code should be refactored to do context.constants.add to make it explicit that we are adding to the context.
There was a problem hiding this comment.
Hm, maybe I should have folded the follow up PR into this. There I add a comment about this.
The interesting thing is that the serialization is able to do it without mutation because we create the mapping from semantic objects to indices, and then use those indices for the syntax objects. So that can go in one batch. And I tried to keep the contexts mirroring each other for serialization and deserialization. So it feels kind of off to initialize with a mutable list for one but not for the other.
We might want to change this code again, because currently the order is non-determenistic:
3f7fd4b to
56d73b7
Compare
56d73b7 to
851504c
Compare
851504c to
c9d07a1
Compare
Organize the serialization/deserialization with a context object: