This is an archive of the discontinued LLVM Phabricator instance.

Rationalise the attribute getter/setter methods on Function and CallSite.
ClosedPublic

Authored by deadalnix on Jun 19 2016, 8:53 PM.

Details

Summary

While woring on mapping attributes in the C API, it clearly appeared that the recent changes in the API on the C++ side left Function and Call/Invoke with an attribute API that grew in an ad hoc manner. This makes it difficult to work with it, because one doesn't know which overloads exists and which do not.

Make sure that getter/setter function exists for both enum and string version. Remove inconsistent getter/setter, unless they have many callsites.

This should make it easier to work with attributes in the future.

This doesn't change how attribute works.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 61232.Jun 19 2016, 8:53 PM
deadalnix retitled this revision from to Rationalise the attribute getter/setter methods on Function and CallSite..
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.
deadalnix updated this revision to Diff 62792.Jul 5 2016, 2:27 PM

rebase/ping

ahatanak added inline comments.
include/llvm/IR/CallSite.h
316

What's the reason you are removing this method, but not removeAttribute(unsigned i, StringRef Kind)? Since you are trying to make the API more consistent, it seems to me that you should remove that too or keep this one?

deadalnix added inline comments.Jul 11 2016, 10:38 AM
include/llvm/IR/CallSite.h
316

There is an addAttribute and this one is defined inconsistently. The fact that some override exists and some other do not depending on what you set attribute on makes it very inconvenient to work with it.

Alternatively, we could define this everywhere, but it didn't seemed to be used enough (one callsite in LLVM and one in clang) to be worth it.

deadalnix added inline comments.Jul 11 2016, 10:41 AM
include/llvm/IR/CallSite.h
312

And as to this one, I just kept it because it is massively used in generated code. I thought removing this one was too much of a disturbance to be worth it.

deadalnix updated this revision to Diff 67465.Aug 9 2016, 11:00 PM

Rebase, ping ping ping ping ?!?

deadalnix updated this revision to Diff 70773.Sep 8 2016, 6:34 PM

Come on guys, it is just cleanup. Can we get that in ?

void accepted this revision.Sep 8 2016, 7:09 PM
void edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 8 2016, 7:09 PM
This revision was automatically updated to reflect the committed changes.