[PATCH] D136400: [llvm-ocaml] Migrate from naked pointers to prepare for OCaml 5

Dmitrii Kosarev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Feb 18 22:09:08 PST 2023


Kakadu added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:705
+  free(Temp);
+  CAMLreturn(Arr);
 }
----------------
alan wrote:
> alan wrote:
> > Kakadu wrote:
> > > This code looks suspicious. If we are going to return an array, why not to try to use `caml_alloc_array` function? 
> > > 
> > The original code uses `caml_alloc_tuple`, so that's what I did as well. I think `caml_alloc_array` (which takes a callback that converts the array elements to OCaml values) would work as well. Thanks for suggesting it.
> I investigated more into [`caml_alloc_array`](https://github.com/ocaml/ocaml/blob/92adb0ff67dea9c14643a1c2eb6eeaf9d48629ef/runtime/alloc.c#L218) and found out that it expects the array to have a 0 sentinel value to mark the end rather than taking the length as an argument. This code can be adapted to use `caml_alloc_array` if it allocates `Length + 1` elements instead and sets the last element to `NULL`. Do the reviewers think this is a change I should make?
Using `caml_alloc_array` is indeed cumbersome. It should be better to left it as it is. I would move allocation of OCaml array closer to the for-loop, to make more obvious that no other allocations are between these two things, but it is minor improvement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136400/new/

https://reviews.llvm.org/D136400



More information about the llvm-commits mailing list