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