Skip to content

Rename line_to_byte_offset -> byte_offset #3877

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.

Sign up for GitHub

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

Jump to bottom
Merged
kddnewton merged 1 commit into from
Jan 27, 2026
Merged

Rename line_to_byte_offset -> byte_offset #3877

kddnewton merged 1 commit into from
Jan 27, 2026
+47 −42

Conversation

@kddnewton
Copy link
Collaborator

@kddnewton kddnewton commented Jan 27, 2026

Also, include the column in here. Hopefully we can do some additional optimizations later.

@kddnewton
Rename line_to_byte_offset -> byte_offset
7759acd 
Also, include the column in here. Hopefully we can do some additional
optimizations later.
eregon
eregon approved these changes Jan 27, 2026
View reviewed changes
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

LGTM

@eregon
Copy link
Member

eregon commented Jan 27, 2026

Hopefully we can do some additional optimizations later.

What are you thinking to? That method looks already pretty much optimal, it's basically just an Array#[].

eregon
eregon reviewed Jan 27, 2026
View reviewed changes
lib/prism/parse_result.rb
offsets[find_line(byte_offset) + 1] || source.bytesize
end

# Return the column number for the given byte offset.
Copy link
Member

@eregon eregon Jan 27, 2026

Choose a reason for hiding this comment

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

Suggested change
# Return the column in bytes for the given byte offset.

Copy link
Member

@eregon eregon Jan 27, 2026

Choose a reason for hiding this comment

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

In general I think we should avoid the term column number, at least I would interpret that as starting at 1 like an editor column. So I'd suggest to use column in bytes/characters/code units.
Happy to make a PR for that if you agree.

Copy link
Collaborator Author

@kddnewton kddnewton Jan 27, 2026

Choose a reason for hiding this comment

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

Sure yeah, happy to see that change.

Copy link
Member

@eregon eregon Jan 27, 2026

Choose a reason for hiding this comment

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

#3880

@kddnewton
Copy link
Collaborator Author

kddnewton commented Jan 27, 2026

Hopefully we can do some additional optimizations later.

What are you thinking to? That method looks already pretty much optimal, it's basically just an Array#[].

Not sure. Since we have both operands, we could potentially cache based on both. I'm not sure if it would be worth it, but I wanted to have both.

@kddnewton kddnewton merged commit 20dc949 into main Jan 27, 2026
65 of 66 checks passed
@kddnewton kddnewton deleted the byte-offset branch January 27, 2026 20:05
@eregon eregon mentioned this pull request Jan 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eregon eregon eregon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kddnewton @eregon