1266402 - Add iteration to EnumSet<T> so that it can be used in range-based for loops

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: enum-set-iterator

EnumSet is going to be really useful for the GC to express sets of AllocKinds in a simpler way than using an array, but it needs a couple of extra features to do this.

First, add begin() and end() so that we can iterate over the contents.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8743843 [details] [diff] [review]
enum-set-iterator

Review of attachment 8743843 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/EnumSet.h
@@ +200,5 @@
>  
> +  class ConstIterator
> +  {
> +    const EnumSet<T>& mSet;
> +    uint32_t mPos;

I don't think we want to allow concurrent mutation of the EnumSet being enumerated.  So add

#include "mozilla/ReentrancyGuard.h"

to the top of the file,

#ifdef DEBUG
  friend class ReentrancyGuard;
  bool mEntered;
#endif

to EnumSet itself, then add a |ReentrancyGuard mGuard;| after |mSet| here, then add |mGuard(aSet)| to the member initialization list, then add |ReentrancyGuard g(*this);| to the start of every non-const method in EnumSet.

@@ +221,5 @@
> +      return !(*this == other);
> +    }
> +
> +    T operator*() {
> +      MOZ_ASSERT(mPos < kMaxBits && mSet.contains(T(mPos)));

Split this into two separate assertions, so if the first is hit, it's unambiguous it was the one hit.

In principle the latter can only be hit with concurrent mutations, but still seems a good assert to have.

::: mfbt/tests/TestEnumSet.cpp
@@ +247,5 @@
> +
> +    birds += GADFLY_PETREL;
> +    birds += TRUE_PETREL;
> +    birds += DIVING_PETREL;
> +    birds += STORM_PETREL;

Mix up the ordering of these, so that we're testing that the ordering exposed by iteration isn't simply order of insertion?

Comment 2

[Jon Coppeard (:jonco)]

Attachment: enum-set-iterator v2

I added a ReentrancyGuard, but that didn't work out because a range-based for
loop constructs two iterator objects, for begin() and end().

Instead I added a version count to EnumSet and added checks in the iterator that this has not changed from its initial value.

Another possibility would be to have a keep count of active readers and assert that this is zero when trying to mutate the enum.  However to allow multiple threads to iterate on a single enum set would require making this atomic and I didn't want to pull in that dependency just for debug checks.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8745355 [details] [diff] [review]
enum-set-iterator v2

Review of attachment 8745355 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, right, we'd have two iterators in flight for this.  No ReentrancyGuard is fine, carry on.  Atomicity-handling seems a little overkill, yeah.

::: mfbt/EnumSet.h
@@ +324,4 @@
>    uint32_t mBitField;
> +
> +#ifdef DEBUG
> +  uint32_t mVersion;

Make this a uint64_t so there's no wraparound concern.  We're debug, we don't care about size that much anyway.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d88619681bc2

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d88619681bc2