-
Notifications
You must be signed in to change notification settings - Fork 170
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 assertion in Parser.Source.to_string_trim
.
#1017
Conversation
b463db0
to
a8342a8
Compare
Looks correct, thanks! Can you also add some tests for the function? |
Laziness got the better of me, but you pushing me to add the test got me to the actual root of the problem so thanks for that:
|
@anuragsoni could you take a look? |
This looks reasonable. The problem with the initial implementation was in the while loop that scans for whitespace leading to let[@inline always] to_string_trim t ~pos ~len =
if
pos < 0
|| t.pos + pos >= t.upper_bound
|| len < 0
|| t.pos + pos + len > t.upper_bound
then
invalid_arg
(Format.asprintf
"Http_parser.Source.substring: Index out of bounds., Requested \
off: %d, len: %d"
pos len);
let last = ref (t.pos + len - 1) in
let pos = ref (t.pos + pos) in
while is_space (String.unsafe_get t.buffer !pos) && !pos < !last do
incr pos
done;
while is_space (String.unsafe_get t.buffer !last) && !last > !pos do
decr last
done;
let len = !last - !pos + 1 in
String.sub t.buffer !pos len |
Maybe your alternative suggestion is a bit more explicit. I don't have a strong preference between the two to be honest, do you have any preference? |
I don't have a strong preference on this! |
Let's merge then, we can always amend this later if we deem it necessary |
Thank you all a lot! |
I'm occasionally getting the assertion below. I didn't try reproducing, but an indexing error in
to_string_trim
is probably the culprit.