| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -728,6 +728,10 @@ def __contains__(self, other): | |
| type(other).__qualname__, self.__class__.__qualname__)) | ||
| return other._value_ & self._value_ == other._value_ | ||
|
|
||
| def __iter__(self): | ||
| members, extra_flags = _decompose(self.__class__, self.value) | ||
| return (m for m in members if m._value_ != 0) | ||
|
|
||
|
Comment thread
Comment on lines
+731
to
+734
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThe _decompose function is heavyweight (fairly complex and includes sorting). I wouldn't expect this kind of overhead when iterating flag bits. I can think of two efficient ways:
return (v for v in self.__class__ if v.value != 0 and (v.value & (v.value-1) == 0) and v in self)
@staticmethod
def _bits(n):
while n:
b = n & (~n+1)
yield b
n ^= b
def __iter__(self):
return (self.__class__(v) for v in self._bits(self._value_))in fact _decompose is used in other places, like __invert__, making these operations really slow. Really the set of available bits should be available in a class-only attribute, like _all_.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Qualityproposal (2) is 2x faster than the implementation in this PR, for an enum with only about 6 values... >>> from enum import Flag, auto
>>> class Foo(Flag):
... NONE = 0
... A = auto()
... B = auto()
... C = auto()
... C2 = C
... AB = A | B
... D = auto()
... E = auto()
... F = auto()
...
>>> x = Foo.A | Foo.B | Foo.D | Foo.F
>>> import timeit
>>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.6332780510000013proposal (2): >>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.36078684099999236
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI have become increasingly unhappy with the the multiple calls to _decompose. Your ideas (and others, too) are welcome.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
I do have ideas on it, and intended to open a separate issue for refactoring _decompose() and make a PR replacing it with something simpler, faster, and less buggy. (E.g. use the _bits() iterator; and I noticed that all callers of it either use the members or extra_flags output, never both; and the computation of extra_flags would be trivial if we had an __all__ attribute on the class.) Regarding bugs, when it's used by repr() or str(), I don't think it's intended that a lone compound value is represented as-is, while a compound plus other value is represented as a redundant "compound plus underlying bits": >>> class Foo(Flag):
... A = auto()
... B = auto()
... C = auto()
... BC = B | C
>>> repr(Foo.BC)
'<Foo.BC: 6>'
>>> repr(Foo.A | Foo.BC)
'<Foo.BC|C|B|A: 7>'
Sorry, something went wrong.
All reactions
Comment thread
Comment on lines
+731
to
+734
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI think the use of __iter__() is problematic given that __contains__() has different semantics. __contains__() returns true for members with value 0 or compound members, and __iter__() does not. It's bad form for these not to match. >>> class Foo(Flag): ... NONE = 0 ... A = 1 ... B = 2 ... C = 4 ... C2 = C ... AB = A | B >>> x = Foo.A | Foo.B >>> list(x) [<Foo.B: 2>, <Foo.A: 1>] >>> Foo.NONE in x True >>> Foo.AB in x True I'd argue that __contains__() is completely broken and no one wants that behavior. But assuming that can't be changed, using __iter__() to iterate the bits is probably not a good idea.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI agree that the zero-value flag should not be contained in a non-zero flag, but I have no problem with a compound flag showing up if its bits are present. You are welcome to try and convince me otherwise, but it will take more than "no one wants that behavior".
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityHmm, I just learned that Flag.__contains__() is technically a subset operator: >>> class Foo(Flag):
... NONE = 0
... A = 1
... B = 2
... C = 4
... AB = A | B
>>> Foo.AB in (Foo.AB | Foo.C)
True
>>> (Foo.A|Foo.C) in (Foo.AB | Foo.C)
TrueIf that's the case, then actually it's OK for Foo.None in Foo.A to be True, since Foo.None is the empty set: >>> set().issubset({1, 2})
TrueI would not have made the API this way. __contains__ should be strictly for testing whether a bit is in the value (a Flag value is a set of enabled bits, in my mind), while issubset() and operator <= should be used for subset.
Sorry, something went wrong.
All reactions
|
||
| def __repr__(self): | ||
| cls = self.__class__ | ||
| if self._name_ is not None: | ||
| Expand Down | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| `enum.Flag` and `enum.IntFlag` members are now iterable |
| Back | FazBrowse Home | New Git URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThe meaning of "members" should be clarified. Specifically, the iteration intentionally omits aliases and compound members.
The tests should verify this.
I would describe it as "iterating over the bits of a flag value", because that's what it does. Using "bits" should be OK since Flag is documented as supporting bitwise operators.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI agree.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.