[PATCH] D39982: [IRBuilder] Set the insert point and debug location together
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 13 20:52:12 PST 2017
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
More information about the llvm-commits
mailing list