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

[Josh Berdine via Phabricator via llvm-commits]

jberdine marked 6 inline comments as done.
jberdine added inline comments.


================
Comment at: lib/IR/Core.cpp:1162
+const char *LLVMGetDebugLocDirectory(LLVMValueRef Val, unsigned *Length) {
+  if (isa<Instruction>(unwrap(Val))) {
+    StringRef S = unwrap<Instruction>(Val)->getDebugLoc()->getDirectory();
----------------
abdulras wrote:
> I think that `unwrap(Val)` should be possible to be hoisted rather than done in each case.
Can further hoisting be done now?


================
Comment at: lib/IR/Core.cpp:1164
+    StringRef S = unwrap<Instruction>(Val)->getDebugLoc()->getDirectory();
+    *Length = S.size();
+    return S.data();
----------------
abdulras wrote:
> Please mark the `Length` parameter as `_Nonnull`, add an assert, or add a check for `Length` being non-null here.
I added an `assert` above instead of `_Nonnull` since this is expected to be called from outside, where an attribute is less likely to be checkable.


================
Comment at: lib/IR/Core.cpp:1188
+  }
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
----------------
hiraditya wrote:
> abdulras wrote:
> > This is going to get "optimized" away in `NDEBUG` builds.  If you hoist the `S` out of the cases, and sink the `Length` assignment and `return`, you at least get predictable behaviour.
> This assert will not trigger in a release build. Is there a better way to handle this?
Do you expect the current version to be so "optimized"? (I'm not positive I grokked what you had in mind with 'sink the `Length` assignment and `return`'.)


Repository:
  rL LLVM

https://reviews.llvm.org/D52210