search

janestreet / core_kernel

Jane Street's standard library overlay (kernel)
MIT License
219 stars 63 forks source link

Add unsafe_index to iobuf #104

Closed anuragsoni closed 3 years ago

anuragsoni commented 3 years ago

Similar to index but returns -1 instead of None to indicate Not_found.

Signed-off-by: Anurag Soni anurag@sonianurag.com

bcc32 commented 3 years ago

Thanks for the patch. I've imported it into our internal code review system.

FYI, unsafe here is really supposed to mean "no bounds checking". It usually also implies that the code using it is carefully tuned for performance and avoiding allocation, so that's why it would return -1 instead of an option. Is that your use case, or do you have some other reason you want a version of this function which returns a negative number?

anuragsoni commented 3 years ago

@bcc32 It usually also implies that the code using it is carefully tuned for performance and avoiding allocation, so that's why it would return -1 instead of an option. Is that your use case

This was indeed the usecase I want. I struggled with name as I believe spos does perform bounds checking. Maybe using https://github.com/janestreet/core_kernel/blob/1b079252ad82677f3f960b951e1aa836d6fb1c91/iobuf/src/iobuf.ml#L433 directly is more appropriate for a function like this? (with appropriate warning in the documentation).

bcc32 commented 3 years ago

Got it. Yes, unsafe_buf_pos sounds like what you want. (I'm making these changes internally, so you don't have to do anything to this PR. It would be updated when we merge it in our next GitHub update.)

anuragsoni commented 3 years ago

I'm making these changes internally, so you don't have to do anything to this PR. It would be updated when we merge it in our next GitHub update.

Thanks!

bcc32 commented 3 years ago

we moved the function to be named Iobuf.Unsafe.Peek.index_or_neg to conform to existing convention in this library. _or_neg is meant as a helpful reminder to the caller, of a property which unsafe itself does not suggest.

anuragsoni commented 3 years ago

we moved the function to be named Iobuf.Unsafe.Peek.index_or_neg to conform to existing convention in this library. _or_neg is meant as a helpful reminder to the caller, of a property which unsafe itself does not suggest.

This sounds very reasonable. I like this name more than unsafe_index!

anuragsoni commented 3 years ago

This can probably be closed now as https://github.com/janestreet/core_kernel/commit/5d5af9a4a7c412c1d92d0e0f1d169511e7888e5e adds this new function!

bcc32 commented 3 years ago

You're right. I thought we had some sort of automation for this but maybe it didn't work. Thank you for contributing to this library!