[PATCH] [ADT] Add a leader_iterator to EquivalenceClasses

[Adam Nemet]

Yeah, I think what you say sounds applicable here.  Let me play with this to try to generalize the filtering capability.

> On Jul 2, 2015, at 9:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> (shooting from the hip a bit - only glanced at the code)
> 
> It'd be good to have a generic filtering iterator abstraction as we already have a few uses for such a thing special cased & could refactor those out. (I'd have to go looking to find the existing examples if none come to mind/are already known to you)
> 
> It could take a function pointer as a non-type template parameter, or a lambda (ugh, stateless lambdas still aren't default constructible, and I'm not sure you can derive from them, so I don't know if you can do the empty base class optimization with them... )
> 
> On Wed, Jul 1, 2015 at 11:53 PM, Adam Nemet <anemet at apple.com <mailto:anemet at apple.com>> wrote:
> Hi dblaikie, dexonsmith,
> 
> Recently we had a bug that was caught during the review where the
> isLeader check was mistakenly omitted from the usual pattern of
> traversing an EC:
> 
>   for (EquivalenceClasses<int>::iterator I = EC.begin(), E = EC.end();
>        I != E; ++I) {
>     if (!I->isLeader()) continue // <----------- this line
>     for (EquivalenceClasses<int>::member_iterator MI = EC.member_begin(I);
>          MI != EC.member_end(); ++MI)
>       ...
> 
> I don't think that anything bad really happens if the check is omitted
> other than some overhead but that made me think if we could have an
> iterator in the outer loop that only traverses the leaders/classes:
> 
>   for (EquivalenceClasses<int>::leader_iterator I = EC.leader_begin(),
>          E = EC.leader_end(); I != E; ++I) {
>     for (EquivalenceClasses<int>::member_iterator MI = EC.member_begin(I);
>          MI != EC.member_end(); ++MI)
>       ...
> 
> EC internally uses a set, so leader_iterator is built on top of the
> set::iterator.  In addition, it also needs the end iterator to avoid
> over-running the set when looking for the leader.
> 
> Let me know if this seems like a good idea and then I will update the
> patch with unit tests.
> 
> http://reviews.llvm.org/D10891 <http://reviews.llvm.org/D10891>
> 
> Files:
>   include/llvm/ADT/EquivalenceClasses.h
> 
> Index: include/llvm/ADT/EquivalenceClasses.h
> ===================================================================
> --- include/llvm/ADT/EquivalenceClasses.h
> +++ include/llvm/ADT/EquivalenceClasses.h
> @@ -15,6 +15,7 @@
>  #ifndef LLVM_ADT_EQUIVALENCECLASSES_H
>  #define LLVM_ADT_EQUIVALENCECLASSES_H
> 
> +#include "llvm/ADT/iterator.h"
>  #include "llvm/Support/DataTypes.h"
>  #include <cassert>
>  #include <cstddef>
> @@ -150,10 +151,23 @@
>      // Only leaders provide anything to iterate over.
>      return member_iterator(I->isLeader() ? &*I : nullptr);
>    }
> +  class leader_iterator;
> +  member_iterator member_begin(leader_iterator I) const {
> +    return member_iterator(&*I);
> +  }
>    member_iterator member_end() const {
>      return member_iterator(nullptr);
>    }
> 
> +  /// Iterate over the leaders (i.e. the equivalence classes).
> +  leader_iterator leader_begin() const {
> +    // Only leaders provide anything to iterate over.
> +    return leader_iterator(TheMapping.begin(), TheMapping.end());
> +  }
> +  leader_iterator leader_end() const {
> +    return leader_iterator(TheMapping.end(), TheMapping.end());
> +  }
> +
>    /// findValue - Return an iterator to the specified value.  If it does not
>    /// exist, end() is returned.
>    iterator findValue(const ElemTy &V) const {
> @@ -276,6 +290,51 @@
>        return Node != RHS.Node;
>      }
>    };
> +
> +  class leader_iterator
> +      : public iterator_facade_base<leader_iterator, std::forward_iterator_tag,
> +                                    const ECValue> {
> +    typedef iterator_facade_base<leader_iterator, std::forward_iterator_tag,
> +                                 const ECValue> super;
> +
> +    iterator Iter;
> +    iterator End;
> +
> +    void findLeader() {
> +      for (; Iter != End; ++Iter)
> +        if (Iter->isLeader())
> +          break;
> +    }
> +
> +  public:
> +    typedef size_t size_type;
> +    typedef typename super::pointer pointer;
> +    typedef typename super::reference reference;
> +
> +    explicit leader_iterator() {}
> +    explicit leader_iterator(iterator First, iterator Last)
> +        : Iter(First), End(Last) {
> +      // Ensure that Iter is pointing at the first leader.
> +      findLeader();
> +    }
> +
> +    leader_iterator &operator++() {
> +      assert(Iter != End && "++'d off the end of the list!");
> +      ++Iter;
> +      // If this is not a leader, keep going.
> +      findLeader();
> +      return *this;
> +    }
> +
> +    reference operator*() const {
> +      assert(Iter != End && "Dereferencing end()!");
> +      return *Iter;
> +    }
> +
> +    bool operator==(const leader_iterator &RHS) const {
> +      return Iter == RHS.Iter;
> +    }
> +  };
>  };
> 
>  } // End llvm namespace
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150702/0b3fd0c8/attachment.html>