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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/IR/Core.cpp:1188
+  }
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
----------------
jberdine wrote:
> 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`'.)
Yes, it isn't unheard of for builds to be done without asserts enabled, so that is a valid concern.


================
Comment at: lib/IR/Core.cpp:1162
+const char *LLVMGetDebugLocDirectory(LLVMValueRef Val, unsigned *Length) {
+  assert(Length && "Length must be non-null");
+  StringRef S;
----------------
Not sure I understand the comment about `_Nonnull` not being able to check-able.


================
Comment at: lib/IR/Core.cpp:1169
+    GV->getDebugInfo(GVEs);
+    if (GVEs.size() == 0) goto No_loc;
+    const DIGlobalVariable *V = GVEs[0]->getVariable();
----------------
The use of the `goto` is unnecessary.  Just do:

```
if (GVEs.Size())
  if (const DIGlobalVariable *DGV = GVEs[0]->getVariable())
    S = DGV->getDirectory();
```


================
Comment at: lib/IR/Core.cpp:1176
+    if (!DI) goto No_loc;
+    S = DI->getDirectory();
+  } else {
----------------
The `goto` is unnecesary.  Just do:

```
if (const DISubprogram *DSP = F->getSubprogram())
  S = DSP->getDirectory();
```


================
Comment at: lib/IR/Core.cpp:1215
+  }
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
----------------
I think that this can be rewritten similar to the function above.


================
Comment at: lib/IR/Core.cpp:1230
+  }
+  assert(0 && "Expected Instruction, GlobalVariable or Function");
+}
----------------
I think that it makes sense to use the same pattern with the conditional assignments.


Repository:
  rL LLVM

https://reviews.llvm.org/D52210