<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div>Yeah, I think what you say sounds applicable here. Let me play with this to try to generalize the filtering capability.</div><div><br class=""></div><div><blockquote type="cite" class=""><div class="">On Jul 2, 2015, at 9:18 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">(shooting from the hip a bit - only glanced at the code)<br class=""><br class="">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)<br class=""><br class="">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... )</div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Jul 1, 2015 at 11:53 PM, Adam Nemet <span dir="ltr" class=""><<a href="mailto:anemet@apple.com" target="_blank" class="">anemet@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi dblaikie, dexonsmith,<br class="">
<br class="">
Recently we had a bug that was caught during the review where the<br class="">
isLeader check was mistakenly omitted from the usual pattern of<br class="">
traversing an EC:<br class="">
<br class="">
for (EquivalenceClasses<int>::iterator I = EC.begin(), E = EC.end();<br class="">
I != E; ++I) {<br class="">
if (!I->isLeader()) continue // <----------- this line<br class="">
for (EquivalenceClasses<int>::member_iterator MI = EC.member_begin(I);<br class="">
MI != EC.member_end(); ++MI)<br class="">
...<br class="">
<br class="">
I don't think that anything bad really happens if the check is omitted<br class="">
other than some overhead but that made me think if we could have an<br class="">
iterator in the outer loop that only traverses the leaders/classes:<br class="">
<br class="">
for (EquivalenceClasses<int>::leader_iterator I = EC.leader_begin(),<br class="">
E = EC.leader_end(); I != E; ++I) {<br class="">
for (EquivalenceClasses<int>::member_iterator MI = EC.member_begin(I);<br class="">
MI != EC.member_end(); ++MI)<br class="">
...<br class="">
<br class="">
EC internally uses a set, so leader_iterator is built on top of the<br class="">
set::iterator. In addition, it also needs the end iterator to avoid<br class="">
over-running the set when looking for the leader.<br class="">
<br class="">
Let me know if this seems like a good idea and then I will update the<br class="">
patch with unit tests.<br class="">
<br class="">
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10891&d=AwMFAg&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=nbyac4ALr5Z7FJrWlyNic8XyTnRSc48PVad-2Qe5_k0&s=mdrmmUGSNHA2qa5zXh-_wjdklCElIyEUinKSSYT3a0I&e=" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D10891</a><br class="">
<br class="">
Files:<br class="">
include/llvm/ADT/EquivalenceClasses.h<br class="">
<br class="">
Index: include/llvm/ADT/EquivalenceClasses.h<br class="">
===================================================================<br class="">
--- include/llvm/ADT/EquivalenceClasses.h<br class="">
+++ include/llvm/ADT/EquivalenceClasses.h<br class="">
@@ -15,6 +15,7 @@<br class="">
#ifndef LLVM_ADT_EQUIVALENCECLASSES_H<br class="">
#define LLVM_ADT_EQUIVALENCECLASSES_H<br class="">
<br class="">
+#include "llvm/ADT/iterator.h"<br class="">
#include "llvm/Support/DataTypes.h"<br class="">
#include <cassert><br class="">
#include <cstddef><br class="">
@@ -150,10 +151,23 @@<br class="">
// Only leaders provide anything to iterate over.<br class="">
return member_iterator(I->isLeader() ? &*I : nullptr);<br class="">
}<br class="">
+ class leader_iterator;<br class="">
+ member_iterator member_begin(leader_iterator I) const {<br class="">
+ return member_iterator(&*I);<br class="">
+ }<br class="">
member_iterator member_end() const {<br class="">
return member_iterator(nullptr);<br class="">
}<br class="">
<br class="">
+ /// Iterate over the leaders (i.e. the equivalence classes).<br class="">
+ leader_iterator leader_begin() const {<br class="">
+ // Only leaders provide anything to iterate over.<br class="">
+ return leader_iterator(TheMapping.begin(), TheMapping.end());<br class="">
+ }<br class="">
+ leader_iterator leader_end() const {<br class="">
+ return leader_iterator(TheMapping.end(), TheMapping.end());<br class="">
+ }<br class="">
+<br class="">
/// findValue - Return an iterator to the specified value. If it does not<br class="">
/// exist, end() is returned.<br class="">
iterator findValue(const ElemTy &V) const {<br class="">
@@ -276,6 +290,51 @@<br class="">
return Node != RHS.Node;<br class="">
}<br class="">
};<br class="">
+<br class="">
+ class leader_iterator<br class="">
+ : public iterator_facade_base<leader_iterator, std::forward_iterator_tag,<br class="">
+ const ECValue> {<br class="">
+ typedef iterator_facade_base<leader_iterator, std::forward_iterator_tag,<br class="">
+ const ECValue> super;<br class="">
+<br class="">
+ iterator Iter;<br class="">
+ iterator End;<br class="">
+<br class="">
+ void findLeader() {<br class="">
+ for (; Iter != End; ++Iter)<br class="">
+ if (Iter->isLeader())<br class="">
+ break;<br class="">
+ }<br class="">
+<br class="">
+ public:<br class="">
+ typedef size_t size_type;<br class="">
+ typedef typename super::pointer pointer;<br class="">
+ typedef typename super::reference reference;<br class="">
+<br class="">
+ explicit leader_iterator() {}<br class="">
+ explicit leader_iterator(iterator First, iterator Last)<br class="">
+ : Iter(First), End(Last) {<br class="">
+ // Ensure that Iter is pointing at the first leader.<br class="">
+ findLeader();<br class="">
+ }<br class="">
+<br class="">
+ leader_iterator &operator++() {<br class="">
+ assert(Iter != End && "++'d off the end of the list!");<br class="">
+ ++Iter;<br class="">
+ // If this is not a leader, keep going.<br class="">
+ findLeader();<br class="">
+ return *this;<br class="">
+ }<br class="">
+<br class="">
+ reference operator*() const {<br class="">
+ assert(Iter != End && "Dereferencing end()!");<br class="">
+ return *Iter;<br class="">
+ }<br class="">
+<br class="">
+ bool operator==(const leader_iterator &RHS) const {<br class="">
+ return Iter == RHS.Iter;<br class="">
+ }<br class="">
+ };<br class="">
};<br class="">
<br class="">
} // End llvm namespace<br class="">
<br class="">
EMAIL PREFERENCES<br class="">
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFAg&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=nbyac4ALr5Z7FJrWlyNic8XyTnRSc48PVad-2Qe5_k0&s=1-VFULwFKg02WSWk-wydIrSR1oOqsAt1d0RhWSltmnI&e=" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br class="">
</blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></body></html>