This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Remove some unclear attribute methods
ClosedPublic

Authored by aeubanks on Aug 23 2021, 11:28 PM.

Details

Summary

To any downstream users broken by this change, please examine your uses
of these methods and see if you can use a better method. For example,
getAttribute(AttributeList::FunctionIndex) => getFnAttr(), or
addAttribute(AttributeList::FirstArgIndex + ArgNo) =>
addParamAttribute(ArgNo). 0 corresponds to ReturnIndex, ~0 corresponds
to FunctionIndex. This may make future cleanups less painful.

I've made the mistake of assuming that these indexes are for parameters
multiple times, but actually they're based off of a weird indexing
scheme AttributeList::AttrIndex where 0 is the return value and ~0 is
the function. Hopefully renaming these methods will make this clearer.
Ideally users should use more specific methods like
AttributeList::getFnAttr().

This touches all relevant methods in AttributeList, CallBase, and Function.

This hopefully will make easier a future change to cleanup AttrIndex. A
previous worry about cleaning up AttrIndex was that too many downstream
users would have to look through all uses of AttrIndex and relevant
attribute method calls to see if anything was unintentionally hardcoded
(e.g. using 0 instead of ReturnIndex). With this change hopefully
downstream users will look at existing usages of these methods and clean
them up.

Diff Detail

Unit TestsFailed

Event Timeline

aeubanks created this revision.Aug 23 2021, 11:28 PM
aeubanks published this revision for review.Aug 24 2021, 10:02 AM
aeubanks added a reviewer: rnk.
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

FWIW, I'm fine with this.

dexonsmith added subscribers: fhahn, bogner, t.p.northover.

Change SGTM. New names seem more clear / less dangerous. @bogner, @fhahn, or @t.p.northover, any thoughts?

BTW, staging this as (at least) two commits will make this easier for downstreams:

  1. Add new APIs and update callers, leaving behind the old names forwarding to the new ones (for a bigger commit you might want to split this step up further)
  2. Remove old APIs

That way a downstream can temporarily revert the "Remove old APIs" commit and continuing merging from main, while updating their own code as a background task. (No need to wait really for "Remove old APIs"; as long as it's an easy-to-revert standalone commit then there are good workflows for downstreams.)

Change SGTM. New names seem more clear / less dangerous. @bogner, @fhahn, or @t.p.northover, any thoughts?

BTW, staging this as (at least) two commits will make this easier for downstreams:

  1. Add new APIs and update callers, leaving behind the old names forwarding to the new ones (for a bigger commit you might want to split this step up further)
  2. Remove old APIs

That way a downstream can temporarily revert the "Remove old APIs" commit and continuing merging from main, while updating their own code as a background task. (No need to wait really for "Remove old APIs"; as long as it's an easy-to-revert standalone commit then there are good workflows for downstreams.)

Renaming these methods downstream should be fairly straightforward and quick, I don't think we need to make it easier for downstream users to adapt to this change.

Actually, I'll go ahead and split out the patches. Part one: D108788.

aeubanks retitled this revision from [NFC] Rename attribute methods that work with indexes to [NFC] Remove some unclear attribute methods.Sep 1 2021, 11:36 AM
rnk accepted this revision.Sep 1 2021, 12:39 PM

lgtm, ship it!

This revision is now accepted and ready to land.Sep 1 2021, 12:39 PM
MaskRay accepted this revision.Sep 1 2021, 1:32 PM
This revision was landed with ongoing or failed builds.Sep 2 2021, 12:48 PM
This revision was automatically updated to reflect the committed changes.