[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-c/Core.h:2832
 LLVMBuilderRef LLVMCreateBuilder(void);
+/** Deprecated: Use LLVMPositionBuilder2 instead. */
 void LLVMPositionBuilder(LLVMBuilderRef Builder, LLVMBasicBlockRef Block,
----------------
echristo wrote:
> I really dislike the "2" moniker on APIs. Seems that if we want a new API that has a new behavior we should do that instead with new and descriptive names.
> 
> LLVMPositionBuilderWithLoc?
WithLoc sounds good to me, I'll update it.


================
Comment at: include/llvm/IR/IRBuilder.h:130
+  /// \deprecated{Please use:
+  ///   setInsertPoint(BasicBlock *, DebugLoc)
+  /// }
----------------
echristo wrote:
> vsk wrote:
> > 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.
> Why are we deprecating a C++ API at all? Just change it.
Deprecating the existing API lets us switch to the new one incrementally, which would be less disruptive. If we don't mind some churn and would rather get this all done in one shot, I'm open to that (& can check in a script). @aprantl, @davide -- thoughts?


https://reviews.llvm.org/D39982