Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a decoding algorithm for index maps #132

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

takikawa
Copy link
Collaborator

@takikawa takikawa commented Sep 27, 2024

Rendered draft

This PR adds a decoding algorithm for index maps along the lines of the existing algorithms for normal source maps.

I'll leave a few comments for cases in the spec where it's not clear what we should do:

  • Error cases for offsets
  • The check for index map overlap

source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated
1. If |offsetColumn| is not a number, [=optionally report an error=].
1. If |previousOffset| is not null,
1. If |previousLastMapping| is not null,
1. If |offsetLine| is less than |previousLastMapping|'s [=decoded mapping/generatedLine=], [=optionally report an error=].
Copy link
Collaborator Author

@takikawa takikawa Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these range checks be "less than or equal to" to be strict inequality rather than allowing equality?

source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Show resolved Hide resolved
@takikawa takikawa force-pushed the index-map-algorithm branch from 9570cc4 to fd1bf64 Compare November 8, 2024 22:22
@takikawa takikawa changed the title Draft: Add a decoding algorithm for index maps Add a decoding algorithm for index maps Nov 8, 2024
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Member

Could you rebase? :)

These aren't linked to so they don't need the tags
  * Fix first line issue
  * Use extend again for new mappings
  * Check the overlap of ranges {lastFirst, lastPrev} and the
    {first, last} of the current source map section instead of
    current more complicated steps.
  * The overlap check also ensures the order of the ranges, beyond
    the ordering of the offsets (which itself doesn't guarantee
    range order)
  * Some editorial changes for the check
Also set a default value so the algorithm continues
An implementation may do a generated->original lookup in an index map
by first searching through the sections by their offset and looking in
only the section where the position is within the range {offset, nextOffset}.
This is only valid if all mappings in a given section are contained in
that range, rather than going into the next section's range.

Also simplifies the check by removing the firstPreviousMapping variable
and its use.

The algorithm checks that the
  (1) the offsets are ordered (in 8g)
  (2) the last mapping of the previous section is before the
      current offset (in 8h)
  (3) it doesn't check that the first mapping is after the
      current offset because that is true by construction
@takikawa takikawa force-pushed the index-map-algorithm branch from 9505a3d to 0d6ffb2 Compare December 10, 2024 23:15
@takikawa
Copy link
Collaborator Author

takikawa commented Dec 10, 2024

I rebased this and made some adjustments. It now checks that mapping ranges are always contained within the section offsets too, which I think is required for the assumptions made in implementations about mappings appearing in a unique bucket.

That means mappings are ordered like this:

previousOffset --- firstMapping --- lastMapping --- nextOffset
prev section  | ..... this is a section .....      | ... next section ...

and so on.

@jridgewell jridgewell self-requested a review December 11, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants