-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Verify that spans point to char boundaries #106192
Conversation
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
8c62263
to
8a37390
Compare
This comment has been minimized.
This comment has been minimized.
8a37390
to
513b0f6
Compare
This comment has been minimized.
This comment has been minimized.
513b0f6
to
b28da92
Compare
This comment has been minimized.
This comment has been minimized.
b28da92
to
e184610
Compare
This comment has been minimized.
This comment has been minimized.
Hello @Nilstrieb! What's the status of this PR? |
i don't know |
It's alright, just wanted to make sure you hadn't forgotten about it! |
e184610
to
58fa309
Compare
This comment has been minimized.
This comment has been minimized.
This makes invalid spans a lot easier to debug. A quick and unscientific benchmarking test revealed that the performance impact of this is small at most.
When the span is empty, it doesn't really have a first character. Even worse, when it's empty at the end of the file, adding a byte offset will make it be out of bounds. So we just return the empty span in these cases.
This is a potentially expensive debug assertion, so I benchmarked could be noise, but it doesn't look like a huge slowdown though i'm still not sure whether I'll ever fix all the issues and merge this |
58fa309
to
bc8aa22
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
This makes invalid spans a lot easier to debug. A quick and unscientific benchmarking test revealed that the performance impact of this is small at most.
I added this for debugging #106191
This PR also fixes a small bug that was found by the debug assertions.