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

Calculate line, col from another Position #414

Open
yodalee opened this issue Sep 5, 2019 · 1 comment

Comments

@yodalee
Copy link

commented Sep 5, 2019

I am writing a parser using Pest, and I use Position to store the token position.
I just got one question about the function line_col of Position.
I see the implementation and it will slice the input string, then iterate over it to find any newline character. The implementation is OK, but I wonder the performance will be worse as the position to the end of input.
I think it is possible that, given another Position, and line, col. Calculate the a new line, col based on the input.
If you think it is valuable to add, I think I can take the issue.

@yodalee yodalee changed the title Calculate Position from another Position Calculate line, col from another Position Sep 5, 2019

@CAD97

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

Hmm, line_col does iterate the entire string.

I've done this for another library: ideally, it should rfind the last \n (e.g. with memchr) and then count \n bytes (e.g. with bytecount) to get the line number. This on its own can be a drastic speedup for late positions, with the cost of adding the two dependencies and maybe a little overhead for early positions.

Even so, it would be useful to offer the incremental version, for truly massive use cases. (Unless you need a massive number of line/col pairs, just working with byte positions and lazily normalizing to line/col for display is quite doable and very performant. A surprisingly big factor is how much space you save by saving approximately a usize per token.

TL;DR line_col can be trivially improved by memchr::memrchr and bytecount::count, and there's room for an incremental version as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.