Page MenuHomePhabricator

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

Authored by jyknight on Jan 10 2019, 1:22 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jyknight created this revision.Jan 10 2019, 1:22 PM
jyknight updated this revision to Diff 181152.Jan 10 2019, 2:07 PM

(Ran clang format; forgot to do before uploading before)

whitequark requested changes to this revision.Jan 10 2019, 7:35 PM
whitequark added inline comments.
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

Please add NameLen as this is the convention for all new C APIs.

This revision now requires changes to proceed.Jan 10 2019, 7:35 PM
whitequark added inline comments.
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

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?

jyknight added inline comments.Jan 13 2019, 2:19 PM
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

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).

whitequark added inline comments.Jan 14 2019, 5:41 AM
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

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.

jyknight marked an inline comment as done.Jan 14 2019, 8:59 AM
jyknight added inline comments.
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

OK, I'll send a separate patch to remove NameLen from those two APIs.

Are you OK with this patch as it stands, then?

whitequark added inline comments.Jan 14 2019, 9:00 AM
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

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.

jyknight marked an inline comment as done.Jan 14 2019, 9:37 AM
jyknight added inline comments.
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

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).

whitequark added inline comments.Jan 14 2019, 9:42 AM
llvm/lib/IR/Core.cpp
3600 ↗(On Diff #181152)

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.

echristo accepted this revision.Jan 14 2019, 10:55 AM

If whitequark is happy I am :)

niravd accepted this revision.Jan 14 2019, 12:09 PM
niravd added a subscriber: niravd.

LGTM

This revision was not accepted when it landed; it landed in state Needs Revision.Jan 14 2019, 1:41 PM
This revision was automatically updated to reflect the committed changes.