-
Notifications
You must be signed in to change notification settings - Fork 106
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
Less exceptions in batVect
#769
Less exceptions in batVect
#769
Conversation
5bd98cb
to
fd44d57
Compare
src/batVect.mli
Outdated
[x] such that [f y] returns [Some x] , where [y] is an element | ||
of [e]. *) | ||
(** [filter_map f v] returns a vect consisting of all elements | ||
[b] such that [f a] returns [Some x] , where [a] is an element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Some b
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
src/batVect.mli
Outdated
@@ -621,6 +628,13 @@ val find : ('a -> bool) -> 'a t -> 'a | |||
@raise Not_found if there is no value that satisfies [p] in the | |||
vect [a]. *) | |||
|
|||
val find_opt : ('a -> bool) -> 'a t -> 'a option | |||
(** [find p a] returns [Some x] if [x] is the first element of vect [a] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should say find p v
instead of find p a
and a
instead of x
for consistency with the other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this should say find_opt
, not find
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in fact the phrasing of this find_opt
is slightly different from the find_opt
above, maybe it would be good to use the same text for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the old variable names here for consistency with the Make variable naming choice (I did not update it when I updated the variables around the normal find
function; arguably this would be nicer, but la flemme.), but I fixed the other issues. thanks!
src/batVect.mli
Outdated
|
||
val find_opt : ('a -> bool) -> 'a t -> 'a option | ||
(** [find_opt p v] returns [Some a], where [a] is the first element | ||
of the vector [v] that satisfies the predicat [p], or [None] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: predicat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
fd44d57
to
9f4cae1
Compare
Github auto-closed this based on my force-push activity, but this was not my intent. I will go ahead and merge. Thanks @cpitclaudel for the review -- I plan to send the |
Implementing BatVect.find_opt makes this more efficient (we have at most one allocation for the result, instead of using exception handlers for each left subtree), but the function is not exposed publicly.
This is a follow-up of #768, eliminating local exceptions for control flow from the BatVect module. As a side-effect, I added a
find_opt : ('a -> bool) -> 'a t -> 'a option
function to the existingfind : ('a -> bool) -> 'a t -> 'a
function.