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

Fix an off-by-1 error in section lookup #507

Merged
merged 1 commit into from
May 30, 2024

Conversation

takikawa
Copy link
Contributor

@takikawa takikawa commented May 7, 2024

This should fix issue #506.

@takikawa takikawa force-pushed the fix-section-lookup-off-by-one branch 2 times, most recently from 27daf1a to 86db47b Compare May 7, 2024 22:10
lib/source-map-consumer.js Outdated Show resolved Hide resolved
lib/source-map-consumer.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@bomsy bomsy left a comment

Choose a reason for hiding this comment

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

Thanks for this patch. Apologies for the delay in reviews. It looks great! I've got a few review comments.

@takikawa takikawa force-pushed the fix-section-lookup-off-by-one branch from 86db47b to 3789341 Compare May 28, 2024 17:07
@takikawa
Copy link
Contributor Author

Thanks for this patch. Apologies for the delay in reviews. It looks great! I've got a few review comments.

Thanks for the suggestions! I've updated the patch now.

Copy link
Collaborator

@bomsy bomsy left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. LGTM!

In an indexed map, the offset line & column are stored 1-based. However, the
lookup for originalPositionFor was not incrementing the 0-based column from the
API argument.
@takikawa takikawa force-pushed the fix-section-lookup-off-by-one branch from 3789341 to 79aa958 Compare May 29, 2024 15:57
@takikawa
Copy link
Contributor Author

There was a linter failure in CI, but should be fixed now (looks like the CI might need approval again to run).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants