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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch enum-set-iterator (obsolete) — Splinter Review
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.
Attachment #8743843 - Flags: review?(jwalden+bmo)
Blocks: 1266406
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?
Attachment #8743843 - Flags: review?(jwalden+bmo) → feedback+
Summary: Add iteration to EnumSet<T> so that it can be used in range-based for loopse → Add iteration to EnumSet<T> so that it can be used in range-based for loops
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.
Attachment #8743843 - Attachment is obsolete: true
Attachment #8745355 - Flags: review?(jwalden+bmo)
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.
Attachment #8745355 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/d88619681bc2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.