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

[Josh Berdine via Phabricator via llvm-commits]

jberdine added a comment.

I have redone my own testing with the latest version of this diff plus changes for the inline comments in this last iteration of review. This test translates each of the bitcode files in llvm-project/llvm/test to a different IR, as well as translating and doing some static analysis on several test programs. This testing did not uncover any issues with this diff.

Additionally, the test was repeated where the translation of the functions of each llvm module was done in parallel (using `Domainslib.Task.parallel_for`). This is not much or a parallel stress test, but it did not encounter any issues.

So these inline comments are the last minor changes, then this can be rebased, and I can commit it if you need.



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:837
+  value Array = caml_alloc_tuple_uninit(Length);
+  for (unsigned I = 0; I < Length; I++) {
+    Field(Array, I) = Val_int(Indices[I]);
----------------
It is pre-existing, but while you are at it, it may be clearer and more uniform to change to `++I`.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:842
-value llvm_get_namedmd(LLVMModuleRef M, value Name) {
-  CAMLparam1(Name);
-  value Nodes = caml_alloc_tuple_uninit(
----------------
This CAMLparam needs to be kept since Name needs to survive the allocation of Nodes.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1145
   LLVMValueRef GlobalVar;
-  if ((GlobalVar = LLVMGetNamedGlobal(M, String_val(Name)))) {
-    if (LLVMGlobalGetValueType(GlobalVar) != Ty)
----------------
Removing these parens triggers `warning: using the result of an assignment as a condition without parentheses [-Wparentheses]`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400