-
Notifications
You must be signed in to change notification settings - Fork 208
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
Add support for Flag enums #599
Changes from 3 commits
ab998d4
fe10e25
7a77dd8
8b166c0
6a41ec1
9eabcd1
4657009
bf88b7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1926,6 +1926,14 @@ The following annotations can be specified using the variable-length | |||||||||
mixed enum types (such as ``Shape.Circle + Color.Red``) are | ||||||||||
permissible. | ||||||||||
|
||||||||||
.. cpp:struct:: is_flag_enum | ||||||||||
|
||||||||||
Indicate that the enumeration may be used with bitwise | ||||||||||
operations. This enables the bitwise operators ``| & ^ ~`` | ||||||||||
with two enumeration as operands. | ||||||||||
The result will an enumeration of the same type. | ||||||||||
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.
Suggested change
The enumeration is the type; the named values are called enumerators. (It is a little bit ambiguous whether 3 is properly an "enumerator" if the defined values are 1 and 2, so I suggest wording so as to sidestep that question.) |
||||||||||
So ``Shape(2) | Shape(1) -> Shepe(3)`. | ||||||||||
mwittgen marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
Function binding | ||||||||||
---------------- | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,6 +15,11 @@ case, both modules must use the same nanobind ABI version, or they will be | |||||||||||||||
isolated from each other. Releases that don't explicitly mention an ABI version | ||||||||||||||||
below inherit that of the preceding release. | ||||||||||||||||
|
||||||||||||||||
Version 2.1.0 (TBA) | ||||||||||||||||
------------------- | ||||||||||||||||
* The :cpp:class:`nb::enum_\<T\>() <enum_>` can now create an``enum.Flag``-derived type | ||||||||||||||||
with flag :cpp:class:`nb::is_flag_enum() <is_flag_enum>`. | ||||||||||||||||
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.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
Version 2.0.1 (TBA) | ||||||||||||||||
--------------------------- | ||||||||||||||||
|
||||||||||||||||
|
@@ -138,9 +143,11 @@ according to `SemVer <http://semver.org>`__. The following changes are | |||||||||||||||
noteworthy: | ||||||||||||||||
|
||||||||||||||||
* The :cpp:class:`nb::enum_\<T\>() <enum_>` binding declaration is now a | ||||||||||||||||
wrapper that creates either a ``enum.Enum`` or ``enum.IntEnum``-derived type. | ||||||||||||||||
wrapper that creates either a ``enum.Enum``, ``enum.IntEnum`` or ``enum.Flag``-derived type. | ||||||||||||||||
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. I think it would make sense to support a matrix of 4 base classes: |
||||||||||||||||
Previously, nanobind relied on a custom enumeration base class that was a | ||||||||||||||||
frequent source of friction for users. | ||||||||||||||||
A new flag :cpp:class:`nb::is_flag_enum() <is_flag_enum>` | ||||||||||||||||
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. This will need to go to a separate changelog entry for 2.1.0 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. Looks like you added the new changelog entry but didn't remove the old one. |
||||||||||||||||
creates a ``enum.Flag``-derived type. | ||||||||||||||||
|
||||||||||||||||
This change may break code that casts entries to integers, which now only | ||||||||||||||||
works for arithmetic (``enum.IntEnum``-derived) enumerations. Replace | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -295,8 +295,11 @@ C++11-style strongly typed enumerations. | |||||||||||||||||
|
||||||||||||||||||
When the annotation :cpp:class:`nb::is_arithmetic() <is_arithmetic>` is | ||||||||||||||||||
passed to :cpp:class:`nb::enum_\<T\> <enum_>`, the resulting Python type | ||||||||||||||||||
will support arithmetic and bit-level operations like comparisons, and, or, | ||||||||||||||||||
xor, negation, etc. | ||||||||||||||||||
will support arithmetic and bit-level operations (and, or, | ||||||||||||||||||
xor, negation). | ||||||||||||||||||
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.
Suggested change
|
||||||||||||||||||
Passing the annotation :cpp:class:`nb::is_flag_enum() <is_flag_enum>` to | ||||||||||||||||||
to :cpp:class:`nb::enum_\<T\> <enum_>`, will result in a Python type | ||||||||||||||||||
`enum.Flags`, that supports bit operations without losing their `Flag` membership. | ||||||||||||||||||
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.
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
.. code-block:: cpp | ||||||||||||||||||
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,9 +62,12 @@ enum class type_flags : uint32_t { | |
is_arithmetic = (1 << 16), | ||
|
||
/// Is the number type underlying the enumeration signed? | ||
is_signed = (1 << 17) | ||
is_signed = (1 << 17), | ||
|
||
// One more flag bits available (18) without needing | ||
/// Is the underlying enumeration type Flag? | ||
is_flag_enum = (1 << 18) | ||
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. Please line this up with all the others. |
||
|
||
// No more flag bits available (18). Needs | ||
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. 🥲 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. When we need this (as we undoubtedly will eventually), suggest splitting the |
||
// a larger reorganization | ||
}; | ||
|
||
|
@@ -201,6 +204,10 @@ NB_INLINE void enum_extra_apply(enum_init_data &e, is_arithmetic) { | |
e.flags |= (uint32_t) type_flags::is_arithmetic; | ||
} | ||
|
||
NB_INLINE void enum_extra_apply(enum_init_data &e, is_flag_enum) { | ||
e.flags |= (uint32_t) type_flags::is_flag_enum; | ||
} | ||
|
||
NB_INLINE void enum_extra_apply(enum_init_data &e, const char *doc) { | ||
e.docstr = doc; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
|
||
#include <Python.h> | ||
#include <frameobject.h> | ||
#include <object.h> | ||
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. I think this include should come with Python.h automatically? |
||
#include <pythread.h> | ||
#include <structmember.h> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
#include "nb_internals.h" | ||
|
||
#include <string> | ||
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. Can we avoid depending on |
||
NAMESPACE_BEGIN(NB_NAMESPACE) | ||
NAMESPACE_BEGIN(detail) | ||
|
||
|
@@ -28,7 +28,7 @@ PyObject *enum_create(enum_init_data *ed) noexcept { | |
handle scope(ed->scope); | ||
|
||
bool is_arithmetic = ed->flags & (uint32_t) type_flags::is_arithmetic; | ||
|
||
bool is_flag_enum = ed->flags & (uint32_t) type_flags::is_flag_enum; | ||
str name(ed->name), qualname = name; | ||
object modname; | ||
|
||
|
@@ -43,7 +43,7 @@ PyObject *enum_create(enum_init_data *ed) noexcept { | |
PyUnicode_FromFormat("%U.%U", scope_qualname.ptr(), name.ptr())); | ||
} | ||
|
||
const char *factory_name = is_arithmetic ? "IntEnum" : "Enum"; | ||
const char *factory_name = (is_arithmetic || is_flag_enum) ? (is_flag_enum ? "Flag" : "IntEnum") : "Enum"; | ||
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. This could be more straightforwardly written as |
||
|
||
object enum_mod = module_::import_("enum"), | ||
factory = enum_mod.attr(factory_name), | ||
|
@@ -56,7 +56,11 @@ PyObject *enum_create(enum_init_data *ed) noexcept { | |
|
||
if (is_arithmetic) | ||
result.attr("__str__") = enum_mod.attr("Enum").attr("__str__"); | ||
|
||
if (is_flag_enum) | ||
result.attr("__str__") = enum_mod.attr("Flag").attr("__str__"); | ||
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. Should this be |
||
#if PY_VERSION_HEX >= 0x030B0000 | ||
result.attr("_boundary_") = enum_mod.attr("FlagBoundary").attr("KEEP"); | ||
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. This choice seems not in keeping with nanobind's general defaults of strictness around enums. I think the default STRICT policy is more appropriate. People can use is_arithmetic (producing an IntFlag) if they want more flexibility. For IntFlag, KEEP is the default. 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. +1 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. See also my point above. We likely want the full matrix of combinations. |
||
#endif | ||
result.attr("__repr__") = result.attr("__str__"); | ||
|
||
type_init_data *t = new type_init_data(); | ||
|
@@ -150,6 +154,28 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8 | |
if (!t) | ||
return false; | ||
|
||
if ((t->flags & (uint32_t) type_flags::is_flag_enum) !=0) { | ||
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. Please follow the code style of the rest of this file. Spaces on both sides of binary operators, space after 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. +1 |
||
auto base = PyObject_GetAttrString((PyObject *)o->ob_type, "__base__"); | ||
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. These Python API calls return new object references which you must drop ( 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. I'm actually wondering if it makes sense to cache these. With flags, it would seem one can generate 2^n cache entries for n bits, which has the potential of being undesirably large. Maybe flags enums are OK to go on a slow path? Or we somehow restrict the size of the cache? 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. AFAICT, they are already being cached in members of the enum type. This "singleton-izes" so that one can test for flag combinations with
The existing cache takes, for each referenced combination, one dict entry (like 32 bytes I think?) plus the footprint of the enum value object itself (sys.getsizeof says 48 bytes, but that doesn't include the 4-entry instance dictionary, name string, etc -- it's probably at least 128 bytes total). We're adding one robin_map entry (24 bytes?) in each direction (C++->Py and Py->C++) for each combination that is passed across the language boundary. So the additional cost of caching is something like 30% above what we're already paying just for using |
||
auto basename = PyObject_GetAttrString(base, "__name__"); | ||
Py_ssize_t size; | ||
const char* data = PyUnicode_AsUTF8AndSize(basename, &size); | ||
std::string s(data, size); | ||
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. This is not a sufficient reason to use Also, I don't think this logic is correct. You should instead check whether
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. +1 |
||
if(s == "Flag") { | ||
auto pValue = PyObject_GetAttrString(o, "value"); | ||
if (pValue == nullptr) { | ||
PyErr_Clear(); | ||
return false; | ||
} | ||
long long value = PyLong_AsLongLong(pValue); | ||
if (value == -1 && PyErr_Occurred()) { | ||
PyErr_Clear(); | ||
return false; | ||
} | ||
*out = (int64_t) value; | ||
return true; | ||
} | ||
} | ||
|
||
enum_map *rev = (enum_map *) t->enum_tbl.rev; | ||
enum_map::iterator it = rev->find((int64_t) (uintptr_t) o); | ||
|
||
|
@@ -175,7 +201,6 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8 | |
} else { | ||
unsigned long long value = PyLong_AsUnsignedLongLong(o); | ||
if (value == (unsigned long long) -1 && PyErr_Occurred()) { | ||
PyErr_Clear(); | ||
return false; | ||
} | ||
enum_map::iterator it2 = fwd->find((int64_t) value); | ||
|
@@ -186,17 +211,19 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8 | |
} | ||
|
||
} | ||
|
||
return false; | ||
} | ||
|
||
PyObject *enum_from_cpp(const std::type_info *tp, int64_t key) noexcept { | ||
type_data *t = nb_type_c2p(internals, tp); | ||
if (!t) | ||
return nullptr; | ||
|
||
enum_map *fwd = (enum_map *) t->enum_tbl.fwd; | ||
|
||
if(t->flags & (uint32_t) type_flags::is_flag_enum) { | ||
PyObject *value = PyLong_FromLongLong(key); | ||
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. This is definitely wrong and the fact that it wasn't caught by a unit test means you need better test coverage. This will turn any flag-enum value into a plain Python int, i.e., it loses its flag-enum membership. That kind of defeats the point of a flag enum, and is extra confusing because you're not allowed to pass that thing back into a C++ function that expects an instance of the flag-enum type. |
||
Py_INCREF(value); | ||
return value; | ||
} | ||
enum_map::iterator it = fwd->find(key); | ||
if (it != fwd->end()) { | ||
PyObject *value = (PyObject *) it->second; | ||
|
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.
Shall we give it a shorter name?
is_flag
? (for symmetry with the arithmetic flag, which is calledis_arithmetic
)