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

[aeubanks]

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.

[shafik]

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]

auto -> unsigned

[rnk]

lgtm

llvm/include/llvm/IR/Attributes.h

854–879

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.

[aeubanks]

add comments

[rnk]

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.

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp

for (auto call_inst : rs_callsites) {
     // add the called function to a set so that we can strip its byval
     // attributes in another pass
     rs_functions.insert(call_inst->getCalledFunction());
 
     // get the function attributes
     llvm::AttributeList call_attribs = call_inst->getAttributes();
 
     // iterate over the argument attributes
-    for (unsigned I = call_attribs.index_begin(); I != call_attribs.index_end();
-         I++) {
+    for (unsigned I : call_attribs.indexes()) {
       // if this argument is passed by val
       if (call_attribs.hasAttributeAtIndex(I, llvm::Attribute::ByVal)) {
         // strip away the byval attribute
         call_inst->removeAttributeAtIndex(I, llvm::Attribute::ByVal);
         changed = true;
       }
     }
   }

llvm/include/llvm/IR/Attributes.h

public:
 
   using iterator = const AttributeSet *;
 
   iterator begin() const;
   iterator end() const;
 
   unsigned getNumAttrSets() const;
 
-  /// Use these to iterate over the valid attribute indices.
-  unsigned index_begin() const { return AttributeList::FunctionIndex; }
-  unsigned index_end() const { return getNumAttrSets() - 1; }
+  // Implementation of indexes(). Produces iterators that wrap an index. Mostly
+  // to hide the awkwardness of unsigned wrapping when iterating over valid
+  // indexes.
+  struct index_iterator {
+    unsigned NumAttrSets;
+    index_iterator(int NumAttrSets) : NumAttrSets(NumAttrSets) {}
+    struct int_wrapper {
+      int_wrapper(unsigned i) : i(i) {}
+      unsigned i;
+      unsigned operator*() { return i; }
+      bool operator!=(const int_wrapper &Other) { return i != Other.i; }
+      int_wrapper &operator++() {
+        // This is expected to undergo unsigned wrapping since FunctionIndex is
+        // ~0 and that's where we start.
+        ++i;
+        return *this;
+      }
+    };
+
+    int_wrapper begin() { return int_wrapper(AttributeList::FunctionIndex); }
+
+    int_wrapper end() { return int_wrapper(NumAttrSets - 1); }
+  };
+
+  /// Use this to iterate over the valid attribute indexes.
+  index_iterator indexes() const { return index_iterator(getNumAttrSets()); }
 
   /// operator==/!= - Provide equality predicates.
   bool operator==(const AttributeList &RHS) const { return pImpl == RHS.pImpl; }
   bool operator!=(const AttributeList &RHS) const { return pImpl != RHS.pImpl; }
 
   /// Return a raw pointer that uniquely identifies this attribute list.
   void *getRawPointer() const {
     return pImpl;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

void ModuleBitcodeWriter::writeAttributeTable() {
   const std::vector<AttributeList> &Attrs = VE.getAttributeLists();
   if (Attrs.empty()) return;
 
   Stream.EnterSubblock(bitc::PARAMATTR_BLOCK_ID, 3);
 
   SmallVector<uint64_t, 64> Record;
   for (unsigned i = 0, e = Attrs.size(); i != e; ++i) {
     AttributeList AL = Attrs[i];
-    for (unsigned i = AL.index_begin(), e = AL.index_end(); i != e; ++i) {
+    for (unsigned i : AL.indexes()) {
       AttributeSet AS = AL.getAttributes(i);
       if (AS.hasAttributes())
         Record.push_back(VE.getAttributeGroupID({i, AS}));
     }
 
     Stream.EmitRecord(bitc::PARAMATTR_CODE_ENTRY, Record);
     Record.clear();
   }

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

void ValueEnumerator::EnumerateAttributes(AttributeList PAL) {
   unsigned &Entry = AttributeListMap[PAL];
   if (Entry == 0) {
     // Never saw this before, add it.
     AttributeLists.push_back(PAL);
     Entry = AttributeLists.size();
   }
 
   // Do lookups for all attribute groups.
-  for (unsigned i = PAL.index_begin(), e = PAL.index_end(); i != e; ++i) {
+  for (unsigned i : PAL.indexes()) {
     AttributeSet AS = PAL.getAttributes(i);
     if (!AS.hasAttributes())
       continue;
     IndexAndAttrSet Pair = {i, AS};
     unsigned &Entry = AttributeGroupMap[Pair];
     if (Entry == 0) {
       AttributeGroups.push_back(Pair);
       Entry = AttributeGroups.size();

llvm/lib/IR/Attributes.cpp

 
 unsigned AttributeList::getNumAttrSets() const {
   return pImpl ? pImpl->NumAttrSets : 0;
 }
 
 void AttributeList::print(raw_ostream &O) const {
   O << "AttributeList[\n";
 
-  for (unsigned i = index_begin(), e = index_end(); i != e; ++i) {
+  for (unsigned i : indexes()) {
     if (!getAttributes(i).hasAttributes())
       continue;
     O << "  { ";
     switch (i) {
     case AttrIndex::ReturnIndex:
       O << "return";
       break;
     case AttrIndex::FunctionIndex:

llvm/lib/Transforms/Utils/FunctionComparator.cpp

int FunctionComparator::cmpMem(StringRef L, StringRef R) const {
   return L.compare(R);
 }
 
 int FunctionComparator::cmpAttrs(const AttributeList L,
                                  const AttributeList R) const {
   if (int Res = cmpNumbers(L.getNumAttrSets(), R.getNumAttrSets()))
     return Res;
 
-  for (unsigned i = L.index_begin(), e = L.index_end(); i != e; ++i) {
+  for (unsigned i : L.indexes()) {
     AttributeSet LAS = L.getAttributes(i);
     AttributeSet RAS = R.getAttributes(i);
     AttributeSet::iterator LI = LAS.begin(), LE = LAS.end();
     AttributeSet::iterator RI = RAS.begin(), RE = RAS.end();
     for (; LI != LE && RI != RE; ++LI, ++RI) {
       Attribute LA = *LI;
       Attribute RA = *RI;
       if (LA.isTypeAttribute() && RA.isTypeAttribute()) {

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

public:
   void visitCallBase(CallBase &I) {
     visitAttributeList(I.getAttributes(), CallsToRefine[&I]);
   }
 
   void visitAttributeList(const AttributeList &AL,
                           AttrPtrVecVecTy &AttributeSetsToPreserve) {
     assert(AttributeSetsToPreserve.empty() && "Should not be sharing vectors.");
     AttributeSetsToPreserve.reserve(AL.getNumAttrSets());
-    for (unsigned SetIdx = AL.index_begin(), SetEndIdx = AL.index_end();
-         SetIdx != SetEndIdx; ++SetIdx) {
+    for (unsigned SetIdx : AL.indexes()) {
       AttrPtrIdxVecVecTy AttributesToPreserve;
       AttributesToPreserve.first = SetIdx;
       visitAttributeSet(AL.getAttributes(AttributesToPreserve.first),
                         AttributesToPreserve.second);
       if (!AttributesToPreserve.second.empty())
         AttributeSetsToPreserve.emplace_back(std::move(AttributesToPreserve));
     }
   }