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

[Stdlib] Implement find_first, find_last for maps and sets #869

Merged
merged 6 commits into from Nov 12, 2016

Conversation

Projects
None yet
@g2p
Contributor

g2p commented Oct 24, 2016

Finds the first or last binding or element that satisfies a monotonic predicate.

See discussion in #665.

@g2p g2p changed the title from Implement find_first, find_last for maps and sets to [Stdlib] Implement find_first, find_last for maps and sets Oct 25, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 31, 2016

Contributor

Making the case for extending the stdlib is always difficult. Can you for instance provide pointers to publicly available code that would benefit from the new functions?

Contributor

alainfrisch commented Oct 31, 2016

Making the case for extending the stdlib is always difficult. Can you for instance provide pointers to publicly available code that would benefit from the new functions?

@bluddy

This comment has been minimized.

Show comment
Hide comment
@bluddy

bluddy Oct 31, 2016

I've had to implement these functions before, and because of type opaqueness, had to copy-paste the entire map implementation. I think this is a valuable addition.

bluddy commented Oct 31, 2016

I've had to implement these functions before, and because of type opaqueness, had to copy-paste the entire map implementation. I think this is a valuable addition.

@yakobowski

This comment has been minimized.

Show comment
Hide comment
@yakobowski

yakobowski Oct 31, 2016

@alainfrisch https://github.com/Frama-C/Frama-C-snapshot/blob/master/src/libraries/stdlib/FCSet.mli, functions nearest_elt_le and nearest_elt_ge, are very similar to what is proposed here.

yakobowski commented Oct 31, 2016

@alainfrisch https://github.com/Frama-C/Frama-C-snapshot/blob/master/src/libraries/stdlib/FCSet.mli, functions nearest_elt_le and nearest_elt_ge, are very similar to what is proposed here.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 31, 2016

Contributor

Thanks @bluddy and @yakobowski .

@yakobowski : Would the addition of the functions proposed here avoid a custom implementation of sets in Frama-C?

Contributor

alainfrisch commented Oct 31, 2016

Thanks @bluddy and @yakobowski .

@yakobowski : Would the addition of the functions proposed here avoid a custom implementation of sets in Frama-C?

@yakobowski

This comment has been minimized.

Show comment
Hide comment
@yakobowski

yakobowski Oct 31, 2016

@alainfrisch I think so, yes. The standard library has caught up with the other changes we used to have (such as merge).

yakobowski commented Oct 31, 2016

@alainfrisch I think so, yes. The standard library has caught up with the other changes we used to have (such as merge).

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Oct 31, 2016

Contributor

Couldn't new functions use options rather than Not_found exceptions ?

Contributor

dbuenzli commented Oct 31, 2016

Couldn't new functions use options rather than Not_found exceptions ?

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Oct 31, 2016

As a user, please, 'something option is so much better than exceptions...

/me would give a lot to have a set of functions for the stdlib like MyMap.get myKey myMap that returned an option instead of throwing exceptions all over the place, plus an Option module that had things like map or andThen and so forth...

OvermindDL1 commented Oct 31, 2016

As a user, please, 'something option is so much better than exceptions...

/me would give a lot to have a set of functions for the stdlib like MyMap.get myKey myMap that returned an option instead of throwing exceptions all over the place, plus an Option module that had things like map or andThen and so forth...

@bluddy

This comment has been minimized.

Show comment
Hide comment
@bluddy

bluddy Oct 31, 2016

Consistency. Unless we deprecate and rebuild the whole stdlib (not a bad idea but not the place to do it), we can't just start using options. Also, doesn't option allocate more than using exceptions? You can't always count on Flambda to eliminate the allocation.

bluddy commented Oct 31, 2016

Consistency. Unless we deprecate and rebuild the whole stdlib (not a bad idea but not the place to do it), we can't just start using options. Also, doesn't option allocate more than using exceptions? You can't always count on Flambda to eliminate the allocation.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Oct 31, 2016

Contributor

Consistency. Unless we deprecate and rebuild the whole stdlib (not a bad idea but not the place to do it), we can't just start using options.

Why not ? I don't think one should enshrine design errors in the name of consistency. There's absolutely no loss here at not being consistent you'll be guided by the types. It only makes the API easier and safer to use.

@OvermindDL1 you might be interested by asetmap.

Contributor

dbuenzli commented Oct 31, 2016

Consistency. Unless we deprecate and rebuild the whole stdlib (not a bad idea but not the place to do it), we can't just start using options.

Why not ? I don't think one should enshrine design errors in the name of consistency. There's absolutely no loss here at not being consistent you'll be guided by the types. It only makes the API easier and safer to use.

@OvermindDL1 you might be interested by asetmap.

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Oct 31, 2016

@OvermindDL1 you might be interested by asetmap.

Lots of useful libraries out there, but I am trying to depend only on stdlib and nothing else for a library that I am building. I keep having to write wrappers around various stdlib things, it is compounding a lot. ^.^

And yes, I find 'something option significantly safer, forces you to at least think about the error cases instead of just trying to remember to Blah.mem first, or whatever lookup test there is in a module, the naming is anything but consistent; it would indeed be very nice to rewrite stdlib from scratch with all modern styles that have been learned over time for efficiency and safety both, release an OCaml 5.0.0 for that.

Why can't Flambda eliminate the allocation? It seems that both option and Result.t would be two very good cases to optimize if not generically then specifically.

EDIT0: Re-making the stdlib after Implicit Modules exist that is, and preferably concurrency and no GIL so everything can be made properly immutable or safe in some way...

EDIT1: Do feel free to ignore me, just hard to resist piping in when I saw a conversation between throwing or option, throwing is very... flow-control disrupting, makes things harder to reason...

OvermindDL1 commented Oct 31, 2016

@OvermindDL1 you might be interested by asetmap.

Lots of useful libraries out there, but I am trying to depend only on stdlib and nothing else for a library that I am building. I keep having to write wrappers around various stdlib things, it is compounding a lot. ^.^

And yes, I find 'something option significantly safer, forces you to at least think about the error cases instead of just trying to remember to Blah.mem first, or whatever lookup test there is in a module, the naming is anything but consistent; it would indeed be very nice to rewrite stdlib from scratch with all modern styles that have been learned over time for efficiency and safety both, release an OCaml 5.0.0 for that.

Why can't Flambda eliminate the allocation? It seems that both option and Result.t would be two very good cases to optimize if not generically then specifically.

EDIT0: Re-making the stdlib after Implicit Modules exist that is, and preferably concurrency and no GIL so everything can be made properly immutable or safe in some way...

EDIT1: Do feel free to ignore me, just hard to resist piping in when I saw a conversation between throwing or option, throwing is very... flow-control disrupting, makes things harder to reason...

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Oct 31, 2016

Contributor

I want find_first to be consistent with find and throw an exception.
Thanks to exception matching, usage is just as simple as matching on an option.
My code that depends on this isn't public yet (these functions are used to implement a B+Tree), but with this and #665 we have at least five users relying on independent reimplementations of similar functions.

Contributor

g2p commented Oct 31, 2016

I want find_first to be consistent with find and throw an exception.
Thanks to exception matching, usage is just as simple as matching on an option.
My code that depends on this isn't public yet (these functions are used to implement a B+Tree), but with this and #665 we have at least five users relying on independent reimplementations of similar functions.

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Oct 31, 2016

Thanks to exception matching, usage is just as simple as matching on an option

Exception matching is nice, but it is still easily ignorable and forgettable, where an option mandates making sure to handle both cases and not worrying about something weird happening, like the callstack suddenly unwinding who knows how high just because of some quickly written code. ^.^

OvermindDL1 commented Oct 31, 2016

Thanks to exception matching, usage is just as simple as matching on an option

Exception matching is nice, but it is still easily ignorable and forgettable, where an option mandates making sure to handle both cases and not worrying about something weird happening, like the callstack suddenly unwinding who knows how high just because of some quickly written code. ^.^

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 1, 2016

Contributor

About Not_found vs option: I tend to be on @dbuenzli side here. Consistency is a nice goal, but being consistent with a bad design for the sake of consistency only does not seem right. A point could be made that if we plan to add option-returning variants with a specific naming scheme (e.g. find_opt, assoc_opt, etc), the new functions should directly follow that naming scheme

Contributor

alainfrisch commented Nov 1, 2016

About Not_found vs option: I tend to be on @dbuenzli side here. Consistency is a nice goal, but being consistent with a bad design for the sake of consistency only does not seem right. A point could be made that if we plan to add option-returning variants with a specific naming scheme (e.g. find_opt, assoc_opt, etc), the new functions should directly follow that naming scheme

@bluddy

This comment has been minimized.

Show comment
Hide comment
@bluddy

bluddy Nov 1, 2016

Wow, @alainfrisch -- that was unexpected.

If anything though, we'd want to label the exception-based functions with _exn as Jane Street does. That would hint that they're less desirable, and convey some information about their usage of exception.

bluddy commented Nov 1, 2016

Wow, @alainfrisch -- that was unexpected.

If anything though, we'd want to label the exception-based functions with _exn as Jane Street does. That would hint that they're less desirable, and convey some information about their usage of exception.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 1, 2016

Contributor

Also, doesn't option allocate more than using exceptions?

Yes, but setting up the exception handler can be costly as well, and you more or less have to do it around each call anyway.

A very quick test with:

let rec find p = function
  | [] -> raise Not_found
  | x :: l -> if p x then x else find p l

let rec find_opt p = function
  | [] -> None
  | x :: l -> if p x then Some x else find_opt p l

let f1 () =
  try find p l
  with Not_found -> 0

let f2 () =
  match find_opt p l with
  | Some x -> x
  | None -> 0

suggests that the find_opt + match..with version is about 50% faster than find + try..with with let p _ = true, and between 0% (for long lists) and 80% faster (for short lists) with let p _ = false.

Contributor

alainfrisch commented Nov 1, 2016

Also, doesn't option allocate more than using exceptions?

Yes, but setting up the exception handler can be costly as well, and you more or less have to do it around each call anyway.

A very quick test with:

let rec find p = function
  | [] -> raise Not_found
  | x :: l -> if p x then x else find p l

let rec find_opt p = function
  | [] -> None
  | x :: l -> if p x then Some x else find_opt p l

let f1 () =
  try find p l
  with Not_found -> 0

let f2 () =
  match find_opt p l with
  | Some x -> x
  | None -> 0

suggests that the find_opt + match..with version is about 50% faster than find + try..with with let p _ = true, and between 0% (for long lists) and 80% faster (for short lists) with let p _ = false.

@bluddy

This comment has been minimized.

Show comment
Hide comment
@bluddy

bluddy Nov 1, 2016

Good to know. Could this be FLambda at work? Because it can't ever optimize setting up exceptions, but it can optimize options being returned if the call is inlined.

bluddy commented Nov 1, 2016

Good to know. Could this be FLambda at work? Because it can't ever optimize setting up exceptions, but it can optimize options being returned if the call is inlined.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Nov 1, 2016

Contributor

If anything though, we'd want to label the exception-based functions with _exn as Jane Street does.

This is quite ugly. Usually you can find better names conveing your intent for that. For example you can have find for the paths where you are unsure something matches (and hence returns an option) and get for the paths where you know by construction something has to match (and raises Invalid_argument if that thing was not in there).

Contributor

dbuenzli commented Nov 1, 2016

If anything though, we'd want to label the exception-based functions with _exn as Jane Street does.

This is quite ugly. Usually you can find better names conveing your intent for that. For example you can have find for the paths where you are unsure something matches (and hence returns an option) and get for the paths where you know by construction something has to match (and raises Invalid_argument if that thing was not in there).

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 2, 2016

Contributor

Could this be FLambda at work?

No, timings were without flambda. Allocating short-lived blocks (such as an immediately deconstructed option) is very cheap (at least with the official OCaml runtime system; Javascript backends are another story).

Contributor

alainfrisch commented Nov 2, 2016

Could this be FLambda at work?

No, timings were without flambda. Allocating short-lived blocks (such as an immediately deconstructed option) is very cheap (at least with the official OCaml runtime system; Javascript backends are another story).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 2, 2016

Contributor

+1 to still having an exception version, but with a name which makes it clear why you'd use it (i.e. find+get) - assuming that it's the case that the exception version with no exception handler is faster than the 'a option version?

Contributor

dra27 commented Nov 2, 2016

+1 to still having an exception version, but with a name which makes it clear why you'd use it (i.e. find+get) - assuming that it's the case that the exception version with no exception handler is faster than the 'a option version?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 2, 2016

Contributor

(#885 proposes to add non-raising variants for existing functions.)

Contributor

alainfrisch commented Nov 2, 2016

(#885 proposes to add non-raising variants for existing functions.)

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Nov 2, 2016

assuming that it's the case that the exception version with no exception handler is faster than the 'a option version?

Most of the time you'd still have something like a MyMap.mem check before a MyMap.get, which would likely be more costly anyway.

OvermindDL1 commented Nov 2, 2016

assuming that it's the case that the exception version with no exception handler is faster than the 'a option version?

Most of the time you'd still have something like a MyMap.mem check before a MyMap.get, which would likely be more costly anyway.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 2, 2016

Contributor

Most - but definitely not all the time. An obvious example being keyword symbols in a parser, and also any situation where you would have one mem call before more than one get call. Either Daniel's suggestion of using Invalid_argument (which is never supposed to be caught) and/or documentation should make it very clear that find_opt is probably what's wanted 95% of the time.

Contributor

dra27 commented Nov 2, 2016

Most - but definitely not all the time. An obvious example being keyword symbols in a parser, and also any situation where you would have one mem call before more than one get call. Either Daniel's suggestion of using Invalid_argument (which is never supposed to be caught) and/or documentation should make it very clear that find_opt is probably what's wanted 95% of the time.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 4, 2016

Contributor

So, assuming #885 is accepted, what should we do here? To have a consistent naming, the new option-returning functions should be called *_opt, but if we don't plan to include the Not-found-raising versions, the suffix would be technically useless.

Contributor

alainfrisch commented Nov 4, 2016

So, assuming #885 is accepted, what should we do here? To have a consistent naming, the new option-returning functions should be called *_opt, but if we don't plan to include the Not-found-raising versions, the suffix would be technically useless.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 4, 2016

Contributor

Given your work in #885, there should obviously be no question that there must be option-returning versions. I also think there's enough here to suggest that there should also be exception-raising versions. The two big questions are:

  1. Should the exception-raising versions raise Not_found or Invalid_argument _. Despite the inconsistency, I think I'd be in favour of Invalid_argument, as it moves in the direction of encouraging correct use: although that's balanced against quite a "gotcha" of catching Not_found for analogous functions elsewhere.
  2. The precise names - for this, while I really prefer Daniel's suggest of intent-based naming (get/find, etc.), I think consistency of naming is probably better, so the option-returning versions should have _opt suffices, and the exception-raising versions be undecorated.
Contributor

dra27 commented Nov 4, 2016

Given your work in #885, there should obviously be no question that there must be option-returning versions. I also think there's enough here to suggest that there should also be exception-raising versions. The two big questions are:

  1. Should the exception-raising versions raise Not_found or Invalid_argument _. Despite the inconsistency, I think I'd be in favour of Invalid_argument, as it moves in the direction of encouraging correct use: although that's balanced against quite a "gotcha" of catching Not_found for analogous functions elsewhere.
  2. The precise names - for this, while I really prefer Daniel's suggest of intent-based naming (get/find, etc.), I think consistency of naming is probably better, so the option-returning versions should have _opt suffices, and the exception-raising versions be undecorated.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 4, 2016

Member

You cannot have a find function raise something else than Not_found. You can have get_first, get_last raising the exception of your choice, and I think that either find_first or find_first_opt would be decent names for a function returning an option. (The second more consistent, the first possibly more modern/whatever.)

Member

gasche commented Nov 4, 2016

You cannot have a find function raise something else than Not_found. You can have get_first, get_last raising the exception of your choice, and I think that either find_first or find_first_opt would be decent names for a function returning an option. (The second more consistent, the first possibly more modern/whatever.)

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 4, 2016

Contributor

Agreed. I'm keeping find_first exactly like find, and will add a variant consistent with #885 if that pull request makes it.

Contributor

g2p commented Nov 4, 2016

Agreed. I'm keeping find_first exactly like find, and will add a variant consistent with #885 if that pull request makes it.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Nov 4, 2016

Contributor

I would simply stick to find_first with option and call it a day.

Contributor

dbuenzli commented Nov 4, 2016

I would simply stick to find_first with option and call it a day.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Nov 4, 2016

Contributor

While I realize that consistency was somewhat ruled out as an argument earlier, I think #885 makes the consistency argument stronger.

Contributor

hcarty commented Nov 4, 2016

While I realize that consistency was somewhat ruled out as an argument earlier, I think #885 makes the consistency argument stronger.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 4, 2016

Contributor

While I realize that consistency was somewhat ruled out as an argument earlier

The point was about not using consistency to perpetuate mistakes. It remains a perfectly relevant argument in general.

Contributor

alainfrisch commented Nov 4, 2016

While I realize that consistency was somewhat ruled out as an argument earlier

The point was about not using consistency to perpetuate mistakes. It remains a perfectly relevant argument in general.

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 9, 2016

Contributor

It's done. I'm keeping both variants. Exception style fits my use case (a btree always has a top key), and the other style is popular as well.

Contributor

g2p commented Nov 9, 2016

It's done. I'm keeping both variants. Exception style fits my use case (a btree always has a top key), and the other style is popular as well.

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 9, 2016

Contributor

@gasche, @dra27, @alainfrisch this is ready for a final look and a merge. Thank you.

Contributor

g2p commented Nov 9, 2016

@gasche, @dra27, @alainfrisch this is ready for a final look and a merge. Thank you.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 9, 2016

Member

(I'll let @alainfrisch make decisions on this PR which I have not followed closely enough.)

Member

gasche commented Nov 9, 2016

(I'll let @alainfrisch make decisions on this PR which I have not followed closely enough.)

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 9, 2016

Contributor

I'm not the best one to make a decision here, since I initially proposed these functions (interface and implementation). Having someone else validate that the interface is ok would be useful.

Contributor

alainfrisch commented Nov 9, 2016

I'm not the best one to make a decision here, since I initially proposed these functions (interface and implementation). Having someone else validate that the interface is ok would be useful.

Show outdated Hide outdated stdlib/map.mli
val find_first_opt: (key -> bool) -> 'a t -> (key * 'a) option
(** [find_first_opt f m], where f is a monotonically increasing function,
returns an option containing the binding of [m] with the lowest key [k]
such that [f k], or the [None] option if no such key exists.

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

Replacing the [None] option with [None] would make this clearer.

@hcarty

hcarty Nov 9, 2016

Contributor

Replacing the [None] option with [None] would make this clearer.

Show outdated Hide outdated stdlib/map.mli
val find_last_opt: (key -> bool) -> 'a t -> (key * 'a) option
(** [find_last_opt f m], where f is a monotonically decreasing function,
returns an option containing the binding of [m] with the highest key [k]
such that [f k], or the [None] option if no such key exists.

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

As above, replacing the [None] option with [None] would make this clearer.

@hcarty

hcarty Nov 9, 2016

Contributor

As above, replacing the [None] option with [None] would make this clearer.

Show outdated Hide outdated stdlib/map.mli
@@ -238,6 +238,34 @@ module type S =
@since 4.05
*)
val find_first: (key -> bool) -> 'a t -> key * 'a
(** [find_first f m], where f is a monotonically increasing function,

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

Show outdated Hide outdated stdlib/map.mli
*)
val find_first_opt: (key -> bool) -> 'a t -> (key * 'a) option
(** [find_first_opt f m], where f is a monotonically increasing function,

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

Show outdated Hide outdated stdlib/map.mli
*)
val find_last: (key -> bool) -> 'a t -> key * 'a
(** [find_last f m], where f is a monotonically decreasing function,

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

Show outdated Hide outdated stdlib/map.mli
*)
val find_last_opt: (key -> bool) -> 'a t -> (key * 'a) option
(** [find_last_opt f m], where f is a monotonically decreasing function,

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

@hcarty

hcarty Nov 9, 2016

Contributor

where f -> where [f]

This comment has been minimized.

@hcarty

hcarty Nov 9, 2016

Contributor

This change would also apply to the Set functions below.

@hcarty

hcarty Nov 9, 2016

Contributor

This change would also apply to the Set functions below.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Nov 9, 2016

Contributor

My apologies for all of the separate comments - I should have started a review to group them together.

Contributor

hcarty commented Nov 9, 2016

My apologies for all of the separate comments - I should have started a review to group them together.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 10, 2016

Member

You could be forgiven by answering Alain's question, giving your external opinion on whether you think this is a good interface. (cc: @c-cube?)

Member

gasche commented Nov 10, 2016

You could be forgiven by answering Alain's question, giving your external opinion on whether you think this is a good interface. (cc: @c-cube?)

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 10, 2016

Contributor

The docstrings were changed per @hcarty's suggestions.

The interface was discussed in #665 as well, I picked the best option from that discussion.

Contributor

g2p commented Nov 10, 2016

The docstrings were changed per @hcarty's suggestions.

The interface was discussed in #665 as well, I picked the best option from that discussion.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Nov 10, 2016

Contributor

I don't have a strong opinion on this interface, it looks ok to me. Maybe it would be good to document more explicitely what "monotonic" is (i.e. if [f k && compare k k' <= 0] holds, then [f k'] must hold), and maybe show a possible use case?

Contributor

c-cube commented Nov 10, 2016

I don't have a strong opinion on this interface, it looks ok to me. Maybe it would be good to document more explicitely what "monotonic" is (i.e. if [f k && compare k k' <= 0] holds, then [f k'] must hold), and maybe show a possible use case?

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 10, 2016

Contributor

I added an example; I think it's enough to illustrate what a monotonic function is, since it's not especially ambiguous.

Contributor

g2p commented Nov 10, 2016

I added an example; I think it's enough to illustrate what a monotonic function is, since it's not especially ambiguous.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Nov 10, 2016

Contributor

@gasche The interface looks OK to me as well. It seems reasonably clear from the type + name of each function what to expect.

Contributor

hcarty commented Nov 10, 2016

@gasche The interface looks OK to me as well. It seems reasonably clear from the type + name of each function what to expect.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 10, 2016

Member

I'll still let @alainfrisch make merge decisions. I don't want to commit to working on the standard-library-extension project, because I have too much on my plate already and it's frustrating work.

Re-reading this PR discussion, it looks like there is agreement that the interface is good, but it's not completely clear to me whether there is a consensus on the name of the exception-raising variants. I would personally be tempted to "let those who do the work decide" and go for @g2p's preference, but stdlib matters are thorny as current choices impact future choices and it's easy to have regrets in the future. Not my call!

@g2p: could you amend the Changelog entry to indicate the people whose review/discussion/proposals helped the PR move forward? You don't need to cite everyone involved in the discussion, but people whose reviewing effort you found substantial and impactful.

Member

gasche commented Nov 10, 2016

I'll still let @alainfrisch make merge decisions. I don't want to commit to working on the standard-library-extension project, because I have too much on my plate already and it's frustrating work.

Re-reading this PR discussion, it looks like there is agreement that the interface is good, but it's not completely clear to me whether there is a consensus on the name of the exception-raising variants. I would personally be tempted to "let those who do the work decide" and go for @g2p's preference, but stdlib matters are thorny as current choices impact future choices and it's easy to have regrets in the future. Not my call!

@g2p: could you amend the Changelog entry to indicate the people whose review/discussion/proposals helped the PR move forward? You don't need to cite everyone involved in the discussion, but people whose reviewing effort you found substantial and impactful.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Nov 10, 2016

Contributor

but it's not completely clear to me whether there is a consensus on the name of the exception-raising variants.

Since exception-raising variants are provided I think the current PR is the best naming scheme you can find.

Contributor

dbuenzli commented Nov 10, 2016

but it's not completely clear to me whether there is a consensus on the name of the exception-raising variants.

Since exception-raising variants are provided I think the current PR is the best naming scheme you can find.

@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 10, 2016

Contributor

Credits added, you're right. Thanks everyone for discussing and reviewing.

Contributor

g2p commented Nov 10, 2016

Credits added, you're right. Thanks everyone for discussing and reviewing.

g2p added some commits Oct 24, 2016

Implement find_first, find_last for maps
Finds the first/last binding where the key satisfies a monotonic
predicate.

Addresses #665, supersedes #868.
Thanks @alainfrisch for the idea and most of the implementation.
Implement find_first, find_last for sets
Finds the first/last element that satisfies a monotonic predicate.
Implement find_first_opt, find_last_opt for maps
Finds the first/last binding where the key satisfies a monotonic
predicate, returning an option.

Followup to #885.
Implement find_first_opt, find_last_opt for sets
Finds the first/last element that satisfies a monotonic predicate,
returning an option.

Followup to #885.
@g2p

This comment has been minimized.

Show comment
Hide comment
@g2p

g2p Nov 12, 2016

Contributor

(rebased to fix a conflict in the change log)

Contributor

g2p commented Nov 12, 2016

(rebased to fix a conflict in the change log)

@alainfrisch alainfrisch merged commit bf4b142 into ocaml:trunk Nov 12, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 12, 2016

Contributor

Thanks to @g2p and everyone involving in the discussion for moving this forward.

Contributor

alainfrisch commented Nov 12, 2016

Thanks to @g2p and everyone involving in the discussion for moving this forward.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

[Stdlib] Implement find_first, find_last for maps and sets (#869)
Finds the first/last binding where the key satisfies a monotonic
predicate.

Addresses #665, supersedes #868.
Thanks @alainfrisch for the idea and most of the implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment