Range mappings: spec text for proposal#233
Conversation
Adds a flag to Decoded Mapping Records which indicate if a mapping is a range mapping. Populate this field based on the decoded range mapping offsets.
| 1. If the result of performing ComparePositions(_last_.[[GeneratedPosition]], _mapping_.[[GeneratedPosition]]) is ~equal~, then | ||
| 1. Append _mapping_.[[OriginalPosition]] to _originalPositions_. | ||
| 1. <ins>If _mapping_.[[IsRangeMapping]] is true, then</ins> |
There was a problem hiding this comment.
(copy-pasting my message from the other PR, to make it more visible)
If we are looking at the mappings at e here:
abcdefg
and we have none but we have two mappings right before at c, out of which one is the range mappings, maybe we should exclude the non-range one? Since the range one actually ends up being an exact match.
Not exactly sure though.
| <td>a non-negative integer</td> | ||
| </tr> | ||
| <tr> | ||
| <td><ins>[[MappingIndex]]</ins></td> |
There was a problem hiding this comment.
Nit: can we call this something with "range mapping"?
| 1. If _relativeIndex_ is 0, then | ||
| 1. Optionally report an error. | ||
| 1. Set _state_.[[MappingIndex]] to _state_.[[MappingIndex]] + _relativeIndex_. | ||
| 1. Let _rangeMappingOffset_ be a new Decoded Range Mapping Offset Record { [[GeneratedLine]]: _state_.[[GeneratedLine]], [[MappingIndex]]: _state_.[[MappingIndex]] - 1 }. |
There was a problem hiding this comment.
Why are we dealing with 1-based indexes?
There was a problem hiding this comment.
WIth 1-based indices the error check is pretty simpler, and it's just to reject 0. In the future we could re-use the 0 value for something if there's a need.
If it were 0-based, we would need to error or no-op if a 0 shows up but it's not the first value in the line.
There was a problem hiding this comment.
Going though this in code, I still hate it. When I'm reading the relative offsets, I don't want to think about 1-indexing. I would rather this be 0-index, and push the complication into error checking.
This is not a strong objection though, because I can just start my relatives at -1 and treat it like it's 0-index anyways. It just hurts my brain to reason about.
There was a problem hiding this comment.
Maybe we can revisit this & discuss in the next TG4 meeting. It's not crucial to the proposal that it be 1-based, so if it's confusing to deal with then we could consider changing it back and just adding the error checks.
* Address some quick fixes from review * Move null check into decode operation * Return an empty List on null range mappings
* Refactor range mapping decoding based on feedback * Range mapping decoding happens separate of and after mapping decoding now * Mappings are updated in-place to add range mapping info * Continue if mapping is NOT-FOUND in range mapping update
| </h1> | ||
| <dl class="header"></dl> | ||
| <emu-alg> | ||
| 1. Let _currentLine_ be *null*. |
There was a problem hiding this comment.
Nit: -1, so we have a consistent number type?
| 1. Else, | ||
| 1. Set _currentIndex_ to _currentIndex_ + 1. | ||
| 1. If _currentLine_ = _offset_.[[GeneratedLine]] and _currentIndex_ = _offset_.[[MappingIndex]], then | ||
| 1. Return _mapping_. |
There was a problem hiding this comment.
Nit:
| 1. Else, | |
| 1. Set _currentIndex_ to _currentIndex_ + 1. | |
| 1. If _currentLine_ = _offset_.[[GeneratedLine]] and _currentIndex_ = _offset_.[[MappingIndex]], then | |
| 1. Return _mapping_. | |
| 1. If _currentLine_ = _offset_.[[GeneratedLine]] and _currentIndex_ = _offset_.[[MappingIndex]], then | |
| 1. Return _mapping_. | |
| 1. Set _currentIndex_ to _currentIndex_ + 1. |
| 1. If _relativeIndex_ is 0, then | ||
| 1. Optionally report an error. | ||
| 1. Set _state_.[[MappingIndex]] to _state_.[[MappingIndex]] + _relativeIndex_. | ||
| 1. Let _rangeMappingOffset_ be a new Decoded Range Mapping Offset Record { [[GeneratedLine]]: _state_.[[GeneratedLine]], [[MappingIndex]]: _state_.[[MappingIndex]] - 1 }. |
There was a problem hiding this comment.
Going though this in code, I still hate it. When I'm reading the relative offsets, I don't want to think about 1-indexing. I would rather this be 0-index, and push the complication into error checking.
This is not a strong objection though, because I can just start my relatives at -1 and treat it like it's 0-index anyways. It just hurts my brain to reason about.
* Change range mapping index encoding to start at 0 Currently "BB" means the first and second mappings are range mappings. Instead, we'll have "AB" encode the same. "BA" will be an (optional) error because a duplicate index is encoded. * Fix detection of duplicate range mapping offsets Disallows a range mapping string like "AA" that wasn't covered by the previous attempt. * Add missing empty RangeMappingLine case * Minor fix: replace not equal sign with "is not"
3 participants
This is the actual PR for the draft range mappings proposal for detailed review. Comments and feedback welcome.
On the other PR #232, Nic raised some points about what we should do in case there are multiple mappings in one location and only one (or a subset) are range mappings.
https://tc39.es/ecma426/pr/233/