Page MenuHomePhabricator

[OCaml] Add (get/set)_module_identifer functions
ClosedPublic

Authored by vaivaswatha on Mar 18 2021, 3:52 AM.

Details

Summary

Also

  1. Fix a bug that crept in when fixing a buildbot failure in https://github.com/llvm/llvm-project/commit/f7be9db6220cb39f0eaa12d2af3abedf0d86c303
  2. Use mlsize_t for cstr_to_string as that is what caml_alloc_string specifies.

Diff Detail

Event Timeline

vaivaswatha created this revision.Mar 18 2021, 3:52 AM
vaivaswatha requested review of this revision.Mar 18 2021, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2021, 3:52 AM
jberdine accepted this revision.Mar 18 2021, 6:23 AM

LGTM

llvm/bindings/ocaml/llvm/llvm.mli
550
llvm/bindings/ocaml/llvm/llvm_ocaml.c
47

It might be more clear to use mlsize_t as that is what caml_alloc_string requires, and push any conversion that might be needed to the clients.

This revision is now accepted and ready to land.Mar 18 2021, 6:23 AM
vaivaswatha edited the summary of this revision. (Show Details)

Use mlsize_t for cstr_to_string as suggested in the review.

Whitespace changes

vaivaswatha marked an inline comment as done.Mar 18 2021, 9:33 AM
jberdine added inline comments.Mar 18 2021, 2:56 PM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

Does this still warn without the cast? I was expecting this cast to be not needed anymore.

vaivaswatha marked an inline comment as done.Mar 18 2021, 6:23 PM
vaivaswatha added inline comments.
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

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.

jberdine added inline comments.Mar 19 2021, 5:04 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

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.

vaivaswatha added inline comments.Mar 19 2021, 6:44 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

Passing unsigned to mlsize_t should be safe right? At most it will be a widening.

jberdine added inline comments.Mar 19 2021, 6:50 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

That is my understanding, yes. The definition of mlsize_t is here which uses uintnat defined here. 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.

vaivaswatha added inline comments.Mar 19 2021, 6:55 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

Shall I go ahead and commit?

jberdine added inline comments.Mar 19 2021, 6:58 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

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.

vaivaswatha added inline comments.Mar 19 2021, 7:02 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
342

Thank you very much for the review @jberdine .

This revision was landed with ongoing or failed builds.Mar 20 2021, 8:37 AM
This revision was automatically updated to reflect the committed changes.