Fold map function for List

[bschommer]

As the title indicates this PR implements the quite common and popular function that combines fold and map. I don't have any strong opinion on the order of arguments and or the name.

[dra27]

A few comments, not including an opinion on whether this should go in:

• Batteries has BatList.fold_left_map (see the discussion when it was added in Add List.fold_map ocaml-batteries-team/batteries-included#734).
• If merged, ListLabels needs updating too.
• Order of arguments should match fold_left (which it does - that's a case for having a strong opinion!)
• Containers also has the function, same signature, but called fold_map
• Base also has the function, with two variants folding_map (which doesn't return the accumulator) and fold_map (which does)

[bschommer]

I took inspiration from the above mentioned implementation, the name is inspired by the recently added filter_map. ListLabels is also updated. As said I don't have a strong opinion on the name or argument order but I would like to have this function included since it is rather common and I implemented it myself often enough to wish it to be in the standard library.

[bschommer]

The failing CI build is due to missing changes, I wanted to wait until the name is fixed before adding them.

[cfcs]

nit, but this test case doesn't check that the map part of fold_map does anything.

[bschommer]

I added some more tests.

[cfcs]

Err, sorry to be back with more annoying bits :)

1. A lot of the other functions document (in their mli docstrings) whether or not they are tail-recursive so people can avoid using them if they have very long lists.
   Would it make sense to add that for this function too (since the call to List.rev makes it non-tail-recursive)?

2. Would there be a case for a fold_map_rev which was tail-recursive?

[bschommer]

1. A lot of the other functions document (in their mli docstrings) whether or not they are tail-recursive so people can avoid using them if they have very long lists.
   Would it make sense to add that for this function too (since the call to List.rev makes it non-tail-recursive)?

List.rev is actually tail-recursive.

[dra27]

Given that we have rev_map it might be nice to have the faster rev_fold_map as well, but that doesn't have to be done here.

[cfcs]

List.rev is actually tail-recursive.

Woops, my bad. Indeed, but it's a bit slower as you noted. :)

[bschommer]

Given that we have rev_map it might be nice to have the faster rev_fold_map as well, but that doesn't have to be done here.

I think if we can agree on the name and arguments of the function, we can actually do even a bit more and add the function to all modules that have a map function.

[bschommer]

I rebased the branch. I still think this is a useful and often used function and would like to have it included in the standard library.

[alainfrisch]

Thanks for the contribution.

I agree this is a useful addition. I have a preference for fold_left_map, which is more coherent with the rest of the Stdlib. Even in Seq where the traversal direction is unambiguous, we have fold_left, not just fold.

Does anyone prefer to keep fold_map or push for something else?

we can actually do even a bit more and add the function to all modules that have a map function.

Indeed, one can even generically transform a map function to an accumulating variant:

let map_accu map f acc l =
  let acc = ref acc in
  let g x =
    let new_acc, y = f !acc x in
    acc := new_acc;
    y
  in
  let r = map g l in
  !acc, r

and this shows that providing the function is "just" a convenience for common cases. But let's keep that for later. I find it much less natural for data structures that implements a map but which don't have a canonical traversal ordering. E.g. Map is borderline (its map function specifies the traversal ordering, so that would be ok-ish), but if we had an Hashtbl.map (copying an hashtable, mapping values, with an unspecified traversal ordering), that wouldn't be so good.

[lpw25]

+1 to naming it fold_left_map. To me fold means fold_right.

[bschommer]

I'm okay with fold_left_map the name then also makes the iteration order more clear.

[alainfrisch]

Suggested change

	@since 4.10.0
	@since 4.11.0

[alainfrisch]

Suggested change

	@since 4.10.0
	@since 4.11.0

[alainfrisch]

To be moved to current working version (=> 4.11).

[alainfrisch]

LGTM. Could you move the change entry to 4.11 and update the since tags accordingly? Then it should be good to merge if nobody complains.

[bschommer]

I cleaned up the history, changed the target version and add @cfcs and @alainfrisch as reviewer.

[alainfrisch]

LGTM, will merge soon if nobody complains.

@bschommer : thanks for your contribution (in general, no need to put efforts rewriting the history since we tend to squash-merge small PRs anyway).

[bschommer]

@bschommer : thanks for your contribution (in general, no need to put efforts rewriting the history since we tend to squash-merge small PRs anyway).

No problem.