[PATCH] D18891: [OCaml] Expose the LLVM diagnostic handler

[Jeroen Ketema via llvm-commits]

jketema added a comment.

Thanks for the feedback so far!


================
Comment at: bindings/ocaml/llvm/llvm.mli:428
@@ +427,3 @@
+val set_diagnostic_handler
+  : llcontext -> (DiagnosticInfo.t -> 'a -> unit) -> 'a -> unit
+
----------------
whitequark wrote:
> There is no need to have an `'a` here since OCaml already has closures; the `context` argument in C is closure emulation in essence.
I had that originally, just wasn't quite sure whether that was desired in this case. I'll remove the `'a`

================
Comment at: bindings/ocaml/llvm/llvm_ocaml.c:187
@@ +186,3 @@
+  if (wrapper == NULL) {
+    llvm_raise(*caml_named_value("Llvm.MemoryError"),
+               LLVMCreateMessage(
----------------
whitequark wrote:
> There is `caml_raise_out_of_memory` and I think we should use it.
I looked for something like that but couldn't find it. Thanks for the pointer.

================
Comment at: test/Bindings/OCaml/linker.ml:21
@@ +20,3 @@
+  Printf.printf
+    "Diagnostic handler called: %s\n" (Llvm.DiagnosticInfo.get_description di)
+
----------------
whitequark wrote:
> Is this (and elsewhere in `test/`) actually used in tests?
It's used in the sense that currently the test crashes, because it tries to read invalid bitcode but no handler is installed. Having diagnostic handlers actually fixes the tests updated in this patch.

It's not used in the sense that there's no `FileCheck` for the printed text. I could make the  handler just be empty instead?


http://reviews.llvm.org/D18891