[PATCH] D39982: [IRBuilder] Set the insert point and debug location together

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:138
+  /// of the specified block, and should have the given debug location.
+  void setInsertPoint(BasicBlock *TheBB, const DebugLoc &Loc) {
+    BB = TheBB;
----------------
aprantl wrote:
> DebugLoc is already a value type, so we shouldn't pass it by reference.
Will fix.


================
Comment at: include/llvm/IR/IRBuilder.h:147
+  ///
+  /// Note: This method is deprecated. Please use:
+  ///   setInsertPoint(Instruction *, const DebugLoc &)
----------------
aprantl wrote:
> out of curiosity: is there a doxygen command such as `\deprecated`?
Yes, I'll switch to that.


================
Comment at: include/llvm/IR/IRBuilder.h:167
+  ///
+  /// Note: This method is deprecated. Please use either:
+  ///   setInsertPoint(BasicBlock *, BasicBlock::iterator, const DebugLoc &)
----------------
aprantl wrote:
> either?
Will fix.


================
Comment at: lib/IR/IRBuilder.cpp:25
+  DebugLoc CurrentLoc =
+      InsertPt ? InsertPt->getDebugLoc() : getCurrentDebugLocation();
+
----------------
aprantl wrote:
> Is falling back to getCurrentDebugLocation() a good idea? This looks like the kind of magic default behavior that this patch tries to get rid of.
No, it's not a good idea :), and it doesn't match what the comment says it does. Thanks for the catch. I added a check in the unit test which would fail if we used getCurrentDebugLocation() here.


https://reviews.llvm.org/D39982