[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:130
+  /// \deprecated{Please use:
+  ///   setInsertPoint(BasicBlock *, DebugLoc)
+  /// }
----------------
davide wrote:
> I'd really love to hear Eric (@echristo) input on this, but I have mixed feelings about deprecating an API and introducing a new API with the same name (and just a different overload).
> It seems like people might still use this wrong, should we choose a different name?
I'm open to changing the name here to make it more explicit, e.g setInsertPointAndDebugLoc.


================
Comment at: lib/IR/Core.cpp:2393
 
+void LLVMPositionBuilder2(LLVMBuilderRef Builder, LLVMBasicBlockRef Block,
+                          LLVMValueRef Instr, LLVMValueRef Loc) {
----------------
davide wrote:
> The *2 variants are probably OK, and other projects seem to use it successfully.
> http://man7.org/linux/man-pages/man2/rename.2.html
> Not sure if we have a precedent, but it doesn't seem a bad path forward.
I've seen this pattern floating around, e.g LLVMParseBitcodeInContext2, clang_createTranslationUnit2, etc. IMO '2' implies 'newer', which is good.


https://reviews.llvm.org/D39982