[PATCH] D56556: [opaque pointer types] Update CallInst creation APIs to consistently accept a callee-type argument.

[whitequark via Phabricator via llvm-commits]

whitequark added inline comments.


================
Comment at: llvm/lib/IR/Core.cpp:3600
+                            LLVMValueRef *Args, unsigned NumArgs,
+                            const char *Name) {
+  FunctionType *FTy = unwrap<FunctionType>(Ty);
----------------
jyknight wrote:
> whitequark wrote:
> > jyknight wrote:
> > > whitequark wrote:
> > > > jyknight wrote:
> > > > > whitequark wrote:
> > > > > > whitequark wrote:
> > > > > > > Please add `NameLen` as this is the convention for all new C APIs.
> > > > > > Actually, now that I think about it, making sure that every `Name` argument has a `NameLen` accompanying would involve deprecating hundreds of functions and will probably never get done. Maybe it's OK to not have `NameLen` in IRBuilder bindings.
> > > > > > 
> > > > > > @deadalnix @echristo your opinion on this?
> > > > > Why do we have this convention now? I missed that discussion.
> > > > > 
> > > > > I actually think we should consistently NOT have a NameLen argument for these APIs creating internal IR names. There's no reason to ever need a zero byte in them -- the names are entirely optional anyways! And having inconsistent APIs is just annoying here.
> > > > >  
> > > > > I see that a couple new functions were _just_ added with a NameLen argument (LLVMBuildIntCast2, LLVMCreateBasicBlockInContext) -- and for both of those, I think that argument should be _removed_ from both, ASAP (especially before the branch, which is this week).
> > > > The discussion happened a while ago (maybe a year?). The point of convention is to make use of LLVM-C through FFI easier, since this is, to my best knowledge, the majority of use of LLVM-C.
> > > > 
> > > > > I actually think we should consistently NOT have a NameLen argument for these APIs creating internal IR names. There's no reason to ever need a zero byte in them -- the names are entirely optional anyways! And having inconsistent APIs is just annoying here.
> > > > 
> > > > I agree. Applying it to IR names was an oversight. Absolutely nothing is gained by IR names with embedded zeroes.
> > > OK, I'll send a separate patch to remove NameLen from those two APIs.
> > > 
> > > Are you OK with this patch as it stands, then?
> > > OK, I'll send a separate patch to remove NameLen from those two APIs.
> > 
> > Thank you!
> > 
> > > Are you OK with this patch as it stands, then?
> > 
> > Yes, this and the other one are fine.
> OK, I'm treating that as an "accepted" for this and D56557 (InvokeInst).
> 
> I'm not sure if you missed them in the flood of emails or just haven't had time to look, but there's also two more similar commits: D56558 (for LoadInst) and D56559 (for GetElementPtrInst).
I have not missed them. I just didn't use "Accept" in phab since it appears to treat an accept from any single reviewer as a global accept, and it feels misleading for these patches. The C API changes look good to me in all four now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56556/new/

https://reviews.llvm.org/D56556