[PATCH] Add iterator and iterator range to PredIteratorCache

[Daniel Berlin]

On Tue, Apr 21, 2015 at 1:14 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Apr 21, 2015 at 1:00 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> Hi chandlerc,
>>
>> This lets us use range based for loops, and generally makes the interface
>> much closer to
>> normal pred iterators.
>>
>> http://reviews.llvm.org/D9169
>>
>> Files:
>>   include/llvm/IR/PredIteratorCache.h
>>
>> Index: include/llvm/IR/PredIteratorCache.h
>> ===================================================================
>> --- include/llvm/IR/PredIteratorCache.h
>> +++ include/llvm/IR/PredIteratorCache.h
>> @@ -14,6 +14,7 @@
>>  #ifndef LLVM_IR_PREDITERATORCACHE_H
>>  #define LLVM_IR_PREDITERATORCACHE_H
>>
>> +#include "llvm/ADT/ArrayRef.h"
>>  #include "llvm/ADT/DenseMap.h"
>>  #include "llvm/ADT/SmallVector.h"
>>  #include "llvm/IR/CFG.h"
>> @@ -32,6 +33,13 @@
>>      /// Memory - This is the space that holds cached preds.
>>      BumpPtrAllocator Memory;
>>    public:
>> +    ArrayRef<BasicBlock *>::iterator begin(BasicBlock *BB) {
>>
>> +      return makeArrayRef(GetPreds(BB), GetNumPreds(BB)).begin();
>> +    }
>> +
>> +    ArrayRef<BasicBlock *>::iterator end(BasicBlock *BB) {
>> +      return makeArrayRef(GetPreds(BB), GetNumPreds(BB)).end();
>> +    }
>
>
> Looks like these could just be:
>
> BasicBlock **begin(BasicBlock *BB) {
>   return GetPreds(BB) + GetNumPreds(BB);
> }

Yes, that is much smarter, thanks.

>
>>
>>
>>      /// GetPreds - Get a cached list for the null-terminated predecessor
>> list of
>>      /// the specified block.  This can be used in a loop like this:
>> @@ -65,6 +73,12 @@
>>        Memory.Reset();
>>      }
>>    };
>> +
>> +  iterator_range<ArrayRef<BasicBlock *>::iterator>
>> +  cached_predecessors(PredIteratorCache &PC, BasicBlock *BB) {
>> +    return make_range(PC.begin(BB), PC.end(BB));
>> +  }
>
>
> And /this/ would be:
>
> ArrayRef<BasicBlock*> cached_predecessors(...) {
>   return makeArrayRef(GetPreds(BB), GetNumPreds(BB));
> }

Yes.

>
> (though I guess GetPreds/GetNumPreds are private to the PredIteratorCache?
> So maybe just exposing the ArrayRef directly from PredIteratorCache and
> avoid ever exposing begin/end?)

No, they are in fact, not private. That's one reason the interface is
so ugly compared to walking preds right now :)

I suspect, in fact, that it should just have

size()
ArrayRef<>

and everything else is an internal detail.