[PATCH] D90831: DebugInfo support for OCaml bindings

[Josh Berdine via Phabricator via llvm-commits]

jberdine accepted this revision.
jberdine added a comment.
This revision is now accepted and ready to land.

Thanks @vaivaswatha for making all these changes, and so promptly!

I agree that the question of making the debuginfo types in the ocaml api safer is leading too far afield. If there was a clear path forward, it would be nice to take it from the beginning before making such changes will break backward compatibility, but the situation is not so clear. Particularly with the current loose types, it would be helpful if the llvm_debuginfo.mli file was documented. For example, it is not obvious that Llvm_debuginfo.get_subprogram should only be called on functions. It would be good if this new api were documented along the lines of the llvm_ocaml.mli file. For example, from

  (** [element_type ty] returns the element type of the pointer, vector, or array
      type [ty]. See the method [llvm::SequentialType::get]. *)
  val element_type : lltype -> lltype

the reader has a chance to know the assumptions on the argument, and the reference to the underlying method is useful for more information.

Otherwise I have made only minor comments that are mostly entirely discretionary.

So this LGTM. I'm happy to give feedback on any documentation, etc. that gets added, but functionally this looks good and doesn't need another round of review.



================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:20
+
+#include "caml/alloc.h"
+#include "caml/custom.h"
----------------
I'm not sure what the preferred approach is, but this is not needed since llvm_ocaml.h includes it.


================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:168
+  LLVMDIFlags Flags = 0;
+  Flags |= map_DIFlag(Int_val(i_Flag));
+  return alloc_diflags(Flags);
----------------
why `|`?


================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:341
+
+CAMLprim value
+llvm_di_location_get_inlined_at(LLVMMetadataRef Location) {
----------------
I guess clang-format should be run


================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:353-359
+  value String;
+  if (Directory) {
+    String = caml_alloc_initialized_string(Len, Directory);
+  } else {
+    String = caml_alloc_string(0);
+  }
+  return String;
----------------
Totally your call, but how you moved ptr_to_option to llvm_ocaml.h makes me think that this code is a good candidate for a small helper function there too.


================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:889
+
+/* llmetadata -> llvariable option */
+CAMLprim value
----------------
If llmetadata type aliases are not added, this comment should be updated. Also, this and the next 2 functions have these comments following the pattern in llvm_ocaml.c, but it would be good if this file was uniform with itself to either have them or not. I personally find them to be useful, but they are a bit of maintenance overhead to keep in sync.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml:11
+
+type llmetadata = Llvm.llmetadata
+
----------------
Your call, but this alias can be removed now that it is not in the mli, and it would be more uniform with the treatment of e.g. Llvm.llvalue.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:477
+
+val instruction_get_debug_loc : Llvm.llvalue -> Llvm.llmetadata option
+
----------------
extremely minor nit, but just note that e.g. llvm_instr_get_opcode uses the 'instr' abbreviation


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:1
+(*===-- llvm_analysis.mli - LLVM OCaml Interface --------------*- OCaml -*-===*
+ *
----------------
jberdine wrote:
> 
seems to still have the wrong file name


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

https://reviews.llvm.org/D90831