[PATCH] D98851: [OCaml] Add (get/set)_module_identifer functions

[Vaivaswatha Nagaraj via Phabricator via llvm-commits]

vaivaswatha added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:342
+  const char *Name = LLVMGetModuleIdentifier(M, &Len);
+  return cstr_to_string(Name, (mlsize_t)Len);
+}
----------------
jberdine wrote:
> vaivaswatha wrote:
> > jberdine wrote:
> > > vaivaswatha wrote:
> > > > jberdine wrote:
> > > > > Does this still warn without the cast? I was expecting this cast to be not needed anymore.
> > > > I didn't get a warning on my system. I added this to make the conversion explicit, because `size_t` can be strictly larger than `mlsize_t` which is defined to be `unsigned long`. I'm not sure if other systems might throw a warning though.
> > > Ok, that's fine. Just note that the rest of this code is not following that convention currently. See e.g. llvm_get_string_attr_value which passes an unsigned Length to caml_alloc_string. Also, caml_string_length returns an mlsize_t which is implicitly converted to size_t in a number of places.
> > Passing `unsigned` to `mlsize_t` should be safe right? At most it will be a widening.
> That is my understanding, yes. The definition of mlsize_t is [here](https://github.com/ocaml/ocaml/blob/trunk/runtime/caml/mlvalues.h#L62) which uses uintnat defined [here](https://github.com/ocaml/ocaml/blob/trunk/runtime/caml/config.h#L141-L158). Note that uintnat is defined conditionally based on the size of pointers. So AFAIU it will be compatible with size_t or ocaml will fail to build.
Shall I go ahead and commit?


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

https://reviews.llvm.org/D98851