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

[whitequark via Phabricator via llvm-commits]

whitequark added inline comments.


================
Comment at: include/llvm-c/Core.h:2832
 LLVMBuilderRef LLVMCreateBuilder(void);
+/** Deprecated: Use LLVMPositionBuilderWithLoc instead. */
 void LLVMPositionBuilder(LLVMBuilderRef Builder, LLVMBasicBlockRef Block,
----------------
vsk wrote:
> whitequark wrote:
> > Why should we deprecate these at all? It's legal to pass Loc = NULL for no location, right? Then why shouldn't we make e.g. `LLVMPositionBuilderBefore(...)` an alias to `LLVMPositionBuilderBeforeWithLoc(..., NULL)` and keep existing code working? Very few frontends in the wild bother with any debug information at all, I don't think it makes sense to break all of them.
> We should deprecate these APIs because they aren't great for building frontends which do emit debug info, and because these APIs will (soon, hopefully) have no C++ counterparts.
> 
> It's legal to pass Loc = NULL, but that rewrite is only behavior-preserving for frontends which emit no debug info. To make the transition easier for these frontends, I can provide an upgrade script which rewrites PositionBuilder* to PositionBuilder*WithLoc(..., NULL). Does that sound OK to you?
No, it does not sound OK because most LLVM frontends aren't written in C, they just use the C interface through FFI. Such a change would be onerous for these frontends for no good reason. I am not convinced that "not having an exact C++ counterpart" is a valid reason for deprecating a C API either.

I would be much more fine with plain out removing `SetCurrentDebugLocation` since that is used by fewer frontends.


https://reviews.llvm.org/D39982