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

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 8 14:26:54 PST 2017


vsk added inline comments.


================
Comment at: include/llvm-c/Core.h:2832
 LLVMBuilderRef LLVMCreateBuilder(void);
+/** Deprecated: Use LLVMPositionBuilderWithLoc instead. */
 void LLVMPositionBuilder(LLVMBuilderRef Builder, LLVMBasicBlockRef Block,
----------------
whitequark wrote:
> 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.
I see only two alternatives to deprecating these C APIs: preserve their behavior or change it s.t they do not forward debug info. There's opposition to a silent behavior change on the mailing list and opposition to deprecating the APIs here.

That leaves the do nothing option: essentially, this patch sans the comments about deprecation. That's what I'll do. I'd be interested in any constructive alternatives you have.



https://reviews.llvm.org/D39982





More information about the llvm-commits mailing list