Out of curiosity, did you test that it is indeed tail-recursive? (How?)

Out of curiosity, did you test that it is indeed tail-recursive? (How?)

Yes, I tested it. The following example terminates with the new code, but results in a stack overflow with the 4.14 Stdlib (tested with ocamlc and ocamlopt):

let h = Hashtbl.create 2 in
  for i = 0 to 1_000_000 do Hashtbl.add h 0 i done;
  Hashtbl.find_all h 0

On my machine, this example terminates correctly on trunk without any additional change, with both ocamlc and ocamlopt.

On my machine, this example terminates correctly on trunk without any additional change, with both ocamlc and ocamlopt.

Isn't that because stacks are heap-allocated on trunk, so we never get stack overflows ?

Out of curiosity, did you test that it is indeed tail-recursive? (How?)

Aren't there warnings 71 and 72 for this ?

Aren't there warnings 71 and 72 for this ?

The warnings do not protect, for example, against a typo in the attribute name.

In OCaml 5 stack limits are quite large, so it's better to explicitly set a low stack limit to run these kind of tests, e.g.

OCAMLRUNPARAM="l=100000" ./mytest.exe

In OCaml 5 stack limits are quite large, so it's better to explicitly set a low stack limit to run these kind of tests, e.g.

OCAMLRUNPARAM="l=100000" ./mytest.exe

Aha! That makes testing easier. On my machine, using trunk and setting 10000 for both the stack limit and the number of iterations in the test code, I get a a stack overflow. Using trunk with the change in Stdlib, I can keep the low stack limit and increase the number of iterations by 100x and the test runs fine.

@ferminr would you like to include a Changes entry for OCaml 5.1, or is it intentional not to?

@ferminr would you like to include a Changes entry for OCaml 5.1, or is it intentional not to?

Ah, yes, I'll add an entry in Changes. What do folks normally do? Add the entry when the PR is first submitted, or wait until it's reviewed and approved?

Does the approval of this PR mean we are now allowed to apply TRMC to List.map and friends?

@nojb I don't see why not, but I find it reassuring that it would be for 5.1 rather than 5.0.