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

[Saleem Abdulrasool via Phabricator via llvm-commits]

abdulras 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();
----------------
I think that `unwrap(Val)` should be possible to be hoisted rather than done in each case.


================
Comment at: lib/IR/Core.cpp:1163
+  if (isa<Instruction>(unwrap(Val))) {
+    StringRef S = unwrap<Instruction>(Val)->getDebugLoc()->getDirectory();
+    *Length = S.size();
----------------
Can't you hoist the `unwrap` here into the conditional?

```
if (const auto *I = unwrap<Instruction>(Val))
```


================
Comment at: lib/IR/Core.cpp:1164
+    StringRef S = unwrap<Instruction>(Val)->getDebugLoc()->getDirectory();
+    *Length = S.size();
+    return S.data();
----------------
Please mark the `Length` parameter as `_Nonnull`, add an assert, or add a check for `Length` being non-null here.


================
Comment at: lib/IR/Core.cpp:1176
+      return nullptr;
+    }
+  } else if (isa<Function>(unwrap(Val))) {
----------------
I think that this is better with an early return:

```
if (GVEs.size() == 0) {
  *Length = 0;
  return nullptr;
}
StringRef S = GVEs[0]->getVariable()->getDirectory();
*Length = S.size();
return S.data();
```


================
Comment at: lib/IR/Core.cpp:1186
+      return nullptr;
+    }
+  }
----------------
Similar with an early return.


================
Comment at: lib/IR/Core.cpp:1188
+  }
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
----------------
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.


================
Comment at: lib/IR/Core.cpp:1219
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
+
----------------
Similar behavioural changes as above.


================
Comment at: lib/IR/Core.cpp:1234
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
+
----------------
Similar behavioural changes.


================
Comment at: lib/IR/Core.cpp:1237-1240
+  Instruction *I = unwrap<Instruction>(Val);
+  assert(I && "Expected Instruction");
+  const DebugLoc &L = I->getDebugLoc();
+  return (L ? L->getColumn() : 0);
----------------
Why not:

```
if (const auto *I = unwrap<Instruction>(Val))
  if (const auto &L = I->getDebugLoc())
    return L->getColumn();
return 0;
```


Repository:
  rL LLVM

https://reviews.llvm.org/D52210