[PATCH] D52210: [LLVM-C] Add C APIs to access DebugLoc info

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: lib/IR/Core.cpp:1162
+const char *LLVMGetDebugLocDirectory(LLVMValueRef Val, unsigned *Length) {
+  assert(Length && "Length must be non-null");
+  StringRef S;
----------------
jberdine wrote:
> compnerd wrote:
> > Not sure I understand the comment about `_Nonnull` not being able to check-able.
> This is an entry point that gets called from other language bindings via their foreign function interfaces, which are unlikely to check `_Nonnull`, or even be able to "see" it since they are essentially written against the ABI.
The `_Nonnull` is a hint to the compiler that the value is non-null.  When building that against a header that declares it, the compiler should ensure that the value being passed is non-null.  The problem is that the assertion can be compiled out (`-DNDEBUG`).  Maybe change this to a `if (!Length) return nullptr`?


================
Comment at: lib/IR/Core.cpp:1176
+  } else {
+    assert(0 && "Expected Instruction, GlobalVariable or Function");
+  }
----------------
Add a `return nullptr`.  The assertion can be compiled out.


================
Comment at: lib/IR/Core.cpp:1183
+const char *LLVMGetDebugLocFilename(LLVMValueRef Val, unsigned *Length) {
+  assert(Length && "Length must be non-null");
+  StringRef S;
----------------
Same as above, either annotate with `_Nonnull` or change this to an early return.


================
Comment at: lib/IR/Core.cpp:1197
+  } else {
+    assert(0 && "Expected Instruction, GlobalVariable or Function");
+  }
----------------
Same as above, add a `return nullptr`.


================
Comment at: lib/IR/Core.cpp:1204
+int LLVMGetDebugLocLine(LLVMValueRef Val) {
+  int L = 0;
+  if (const auto *I = unwrap<Instruction>(Val)) {
----------------
Please change this to `unsigned` to match the return type of `DebugLoc::getLine`.


================
Comment at: lib/IR/Core.cpp:1217
+  } else {
+    assert(0 && "Expected Instruction, GlobalVariable or Function");
+  }
----------------
Please add a `return -1` here since the `assert` can be compiled out.


================
Comment at: lib/IR/Core.cpp:1223
+int LLVMGetDebugLocColumn(LLVMValueRef Val) {
+  int C = 0;
+  if (const auto *I = unwrap<Instruction>(Val))
----------------
Please change this to an `unsigned` to match the return type of `DebugLoc::getColumn`.


Repository:
  rL LLVM

https://reviews.llvm.org/D52210