This is an archive of the discontinued LLVM Phabricator instance.

[NFC][AttributeList] Replace index_begin/end with an iterator
ClosedPublic

Authored by aeubanks on Sep 30 2021, 2:18 PM.

Details

Summary

We expose the fact that we rely on unsigned wrapping to iterate through
all indexes. This can be confusing. Rather, keeping it as an
implementation detail through an iterator is less confusing and is less
code.

Event Timeline

aeubanks created this revision.Sep 30 2021, 2:18 PM
aeubanks requested review of this revision.Sep 30 2021, 2:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 30 2021, 2:18 PM
aeubanks retitled this revision from [AttributeList] Replace index_begin/end with an iterator to [NFC][AttributeList] Replace index_begin/end with an iterator.Sep 30 2021, 2:19 PM
aeubanks added reviewers: rnk, nikic.
shafik added a subscriber: shafik.Sep 30 2021, 3:09 PM
shafik added inline comments.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
838

You changed this one to auto but left the others unsigned. I think leaving it as unsigned makes more sense since it is not obvious what we expect the type to be here.

aeubanks updated this revision to Diff 376381.Sep 30 2021, 3:12 PM

auto -> unsigned

rnk accepted this revision.Sep 30 2021, 7:18 PM

lgtm

llvm/include/llvm/IR/Attributes.h
854

This deserves a comment just for consistency with the rest of these top-level members, even though it's a boring implementation detail of indexes().

863

It's probably worth a comment that this operation is expected to overflow & wrap.

This revision is now accepted and ready to land.Sep 30 2021, 7:18 PM
aeubanks updated this revision to Diff 376420.Sep 30 2021, 9:04 PM

add comments

rnk added a comment.Oct 5 2021, 3:25 PM

I had another idle thought on this matter: The unsigned overflow here really isn't that weird. It's the same thing as iterating over the range [-1, 0, #args+1]. We could update all these APIs to traffic in plain signed ints, and then there would be no wrapping, just normal index math.