-
-
Notifications
You must be signed in to change notification settings - Fork 30.2k
New issue
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
bpo-32218: make Flag
and IntFlag
members iterable
#22221
Conversation
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.
IMHO, we should notice this feature on whatsnews for 3.10.
News entry added. |
this PR should have cited https://bugs.python.org/issue38250 |
:class:`Flag` members can also be iterated over:: | ||
|
||
>>> purple = Color.RED | Color.BLUE | ||
>>> list(purple) | ||
[<Color.BLUE: 2>, <Color.RED: 1>] |
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.
The 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.
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.
I agree.
def __iter__(self): | ||
members, extra_flags = _decompose(self.__class__, self.value) | ||
return (m for m in members if m._value_ != 0) | ||
|
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.
The _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:
- compare to all members of the Flag class, excluding 0 and non-power-of-2:
return (v for v in self.__class__ if v.value != 0 and (v.value & (v.value-1) == 0) and v in self)
- walk through the bits with a helper
@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_
.
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.
proposal (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.6332780510000013
proposal (2):
>>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.36078684099999236
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.
I have become increasingly unhappy with the the multiple calls to _decompose
. Your ideas (and others, too) are welcome.
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.
I have become increasingly unhappy with the the multiple calls to _decompose. Your ideas (and others, too) are welcome.
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>'
def __iter__(self): | ||
members, extra_flags = _decompose(self.__class__, self.value) | ||
return (m for m in members if m._value_ != 0) | ||
|
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.
I 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.
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.
I 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".
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.
Hmm, 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)
True
If 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})
True
I 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.
also, since
>>> class Foo(Flag):
... NONE = 0
... A = auto()
... B = auto()
... C = auto()
... AB = A | B
... ABC = A | B | C
>>> list(Foo.ABC)
[<Foo.C: 4>, <Foo.AB: 3>, <Foo.B: 2>, <Foo.A: 1>] |
Yeah, I had recently noticed that myself. Only the one-bit flags should be in the iteration. |
I'm not sure why Flag allows definitions that cannot be decomposed into single-bit members. >>> class Bizarre(Flag):
... b = 3
... c = 4
... d = 6
...
Bizarre
>>> list(Bizarre(7))
[<Bizarre.d: 6>, <Bizarre.c: 4>, <Bizarre.b: 3>] The iterator is no longer enumerating bits, and this defies any kind of efficient implementation of Flags based on bits (like trivial decomposition and calculation of extra_args). I propose that Flag raise ValueError on such a definition. Supporting it is not worth the code complexity and efficiency cost. |
and this is nonsensical: >>> ~Bizarre.d
<Bizarre.0: 0> Other than making such definitions invalid, another approach is to use pseudo members to fill in missing bits. But they should be applied uniformly across the constructor, operations, and repr: >>> class Bizarre(Flag):
... b = 3
... c = 4
... d = 6
...
Bizarre
>>> list(Bizarre(7))
[<Bizarre.c: 4>, <Bizarre.2: 2>, <Bizarre.1: 1>]
>>> Bizarre(2)
<Bizarre.2: 2>
>>> Bizarre(8)
ValueError: 8 is not a valid Bizarre
>>> ~Bizarre.d
<Bizarre.1: 1> |
>>> class Foo(Flag):
... X = 1
...
>>> Foo(1)
<Foo.X: 1>
>>> Foo(-1)
<Foo.X: 1>
>>> Foo(-2)
<Foo.0: 0>
>>> Foo(-3)
ValueError: -3 is not a valid Foo I see the implementation in I don't think Flag should allow negative numbers. |
summary of Flag bugs to file:
|
Member and member combinations can now be iterated over:
https://bugs.python.org/issue32218