[PATCH] D90831: DebugInfo support for OCaml bindings

[Josh Berdine via Phabricator via llvm-commits]

jberdine added a comment.

Looks good, thanks! I made a few very minor comments. I agree that the current state of clang-tidy is good. I don't know how to let it know about the OCaml-specific headers, and that e.g. `value` is a type declared there and it's capitalization is not up for discussion.



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:198
+  Llvm.llmetadata
+(** [dibuild_create_module] Creates a new descriptor for a module with the
+    specified parent scope. See LLVMDIBuilderCreateModule. *)
----------------



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:207
+  Llvm.llmetadata
+(** [dibuild_create_namespace] Creates a new descriptor for a namespace with
+    the specified parent scope. See LLVMDIBuilderCreateNameSpace *)
----------------



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:244
+  Llvm.llmetadata
+(** [dibuild_create] Creates a new DebugLocation that describes a source
+    location. See LLVMDIBuilderCreateDebugLocation *)
----------------



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:247-273
+val di_location_get_line : location:Llvm.llmetadata -> int
+(** [di_location_get_line l] Get the line number of debug location [l]. *)
+
+val di_location_get_column : location:Llvm.llmetadata -> int
+(** [di_location_get_column l] Get the column number of debug location [l]. *)
+
+val di_location_get_scope : location:Llvm.llmetadata -> Llvm.llmetadata
----------------
These 8 don't mention their LLVM-C counterparts


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:259
+  location:Llvm.llmetadata -> Llvm.llmetadata option
+(** [di_location_get_inlined_at l] Get the "inline at" location associated with
+    debug location [l], if it exists. *)
----------------



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:277
+  lldibuilder -> data:Llvm.llmetadata array -> Llvm.llmetadata
+(** [ dibuild_get_or_create_type_array] Create a type array.
+    See LLVMDIBuilderGetOrCreateTypeArray *)
----------------



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:280-292
+val di_global_variable_expression_get_variable :
+  Llvm.llmetadata -> Llvm.llmetadata option
+(** [di_global_variable_expression_get_variable gve] returns the debug variable
+    of [gve], which must be a [DIGlobalVariableExpression].
+    See the [llvm::DIGlobalVariableExpression::getVariable()] method. *)
+
+val di_variable_get_line : Llvm.llmetadata -> int
----------------
Maybe change these to refer to the C apis instead of the C++ ones for consistency with the rest of this module.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:443-445
+(** [dibuild_create_object_pointer_type dib ty] Create a uniqued DIType* clone
+  with FlagObjectPointer and FlagArtificial set. [dib] is the dibuilder
+  value and [ty] the underlying type to which this pointer points. *)
----------------
Mention LLVMDIBuilderCreateObjectPointerType?


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:447-460
+val dibuild_create_qualified_type :
+  lldibuilder -> tag:int -> Llvm.llmetadata -> Llvm.llmetadata
+(** [dibuild_create_qualified_type dib tag ty] Create debugging information
+    entry for a qualified type, e.g. 'const int'. [dib] is the dibuilder value,
+    [tag] identifyies the type and [ty] is the base type. *)
+
+val dibuild_create_reference_type :
----------------
These also don't mention their LLVM-C counterparts.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:551-593
+val dibuild_create_artificial_type :
+  lldibuilder -> ty:Llvm.llmetadata -> Llvm.llmetadata
+(** [dibuild_create_artificial_type dib ty] Create a uniqued DIType* clone with
+    FlagArtificial set.
+    [dib] is the dibuilder value and [ty] the underlying type. *)
+
+val di_type_get_name : Llvm.llmetadata -> string
----------------
These don't mention their LLVM-C counterparts.


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

https://reviews.llvm.org/D90831