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

[Josh Berdine via Phabricator via llvm-commits]

jberdine 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);
+}
----------------
vaivaswatha wrote:
> 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?
Yes, go ahead. I did not mean to imply that there is a problem, only that having the explicit cast here is not the same convention as currently used in the rest of the file, where such casts are left implicit. Your call.


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

https://reviews.llvm.org/D98851