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

[Josh Berdine via Phabricator via llvm-commits]

jberdine added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:46
 
+value to_val(void *ptr) {
+  assert((((value)ptr) & 1) == 0 &&
----------------
alan wrote:
> jberdine wrote:
> > It might not hurt to add a comment before `to_val` and `from_val` saying that they encode pointers to LLVM objects as OCaml tagged integers, following the 3rd encoding suggested by the OCaml FFI documentation at https://ocaml.org/releases/5.0/htmlman/intfc.html#ss:c-outside-head . I don't know how stable such urls are expected to be though.
> I added comments to the header file explaining how these functions work. Should I put additional comments in the source file?
The comment in the header is good but I think since it discusses the implementation details of the encoding, I would have put it in the .c and used a comment in the header that just said that the functions implement an encoding without specifying which one. But I have not checked if the uses in other files need to know about details of the encoding. Those are the things I think of, but go with your preference.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1950-1962
+value llvm_call_site_attrs(value F, value Index) {
+  CAMLparam0();
+  CAMLlocal2(Temp, Array);
+  unsigned Length = LLVMGetCallSiteAttributeCount(Value_val(F), Int_val(Index));
+  Temp = caml_alloc(1, Abstract_tag);
+  Array = caml_alloc_tuple_uninit(Length);
+  LLVMGetCallSiteAttributes(Value_val(F), Int_val(Index),
----------------
You're right about `F` and `Index`. I would expect an implementation more like in the suggested edit, similar to `llvm_struct_element_types`. In particular, `Temp` is an ocaml block with enough space for only 1 word, and I think `LLVMGetCallSiteAttributes` expects a pointer to an array with space for `Length` pointers.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.h:39
+   are 2-byte aligned: to_val sets the low bit so that OCaml treats the value
+   as an integer, and from_val clears the low bit. */
+value to_val(void *ptr);
----------------



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.h:46
+   the representation described above and returns a malloc'd array
+   of unencoded LLVM references. The returned array must be deallocated using
+   free. */
----------------
maybe


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400