This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Add a leader_iterator to EquivalenceClasses
Needs ReviewPublic

Authored by anemet on Jul 1 2015, 11:52 PM.

Details

Summary

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.

Diff Detail

Event Timeline

anemet updated this revision to Diff 28935.Jul 1 2015, 11:52 PM
anemet retitled this revision from to [ADT] Add a leader_iterator to EquivalenceClasses.
anemet updated this object.
anemet edited the test plan for this revision. (Show Details)
anemet added reviewers: dblaikie, dexonsmith.
anemet added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Jul 2 2015, 9:18 AM
dblaikie added a subscriber: dblaikie.

(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... )

dexonsmith resigned from this revision.Aug 25 2019, 8:28 AM