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

[James Y Knight via Phabricator via llvm-commits]

jyknight marked an inline comment as done.
jyknight added inline comments.


================
Comment at: llvm/lib/IR/Core.cpp:3600
+                            LLVMValueRef *Args, unsigned NumArgs,
+                            const char *Name) {
+  FunctionType *FTy = unwrap<FunctionType>(Ty);
----------------
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).


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

https://reviews.llvm.org/D56556