blender
/
blender
134
424
Fork 606
Code Issues 6.3k Pull Requests 769 Projects 19 Wiki Activity

RNA: Add missing raw types for DNA types #115761

Merged
Brecht Van Lommel merged 7 commits from Mysteryem/blender:fix_missing_raw_types_pr into main 2024-01-10 18:19:33 +01:00
Conversation 6 Commits 7 Files Changed 6 +59 -3
3 changed files with 59 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 75a7c4ff49 - Show all commits

45
source/blender/python/generic/py_capi_utils.cc
View File

@ -1636,6 +1636,51 @@ bool PyC_RunString_AsString(const char *imports[],
# pragma GCC diagnostic ignored "-Wtype-limits"
#endif
ulong PyC_Long_AsUnsignedLong(PyObject *value)
{
if (value == nullptr) {
/* Let PyLong_AsUnsignedLong handle error raising. */
return PyLong_AsUnsignedLong(value);
}
if (PyLong_Check(value)) {
return PyLong_AsUnsignedLong(value);
}
/* Call `__index__` like PyLong_AsLong. */
PyObject *value_converted = PyNumber_Index(value);
if (value_converted == nullptr) {
/* A `TypeError` will have been raised. */
return ulong(-1);
}
ulong to_return = PyLong_AsUnsignedLong(value_converted);
Py_DECREF(value_converted);
return to_return;
}
unsigned long long PyC_Long_AsUnsignedLongLong(PyObject *value)
{
if (value == nullptr) {
/* Let PyLong_AsUnsignedLongLong handle error raising. */
return PyLong_AsUnsignedLongLong(value);
}
if (PyLong_Check(value)) {
return PyLong_AsUnsignedLongLong(value);
}
/* Call `__index__` like PyLong_AsLongLong. */
PyObject *value_converted = PyNumber_Index(value);
if (value_converted == nullptr) {
/* A `TypeError` will have been raised. */
return unsigned long long(-1);
Thomas Barlow commented 2023-12-20 22:09:50 +01:00
Outdated
Review
Copy Link

I didn't know that functional-style casts can only be done with simple type specifiers, and for some reason it does compile on Windows.

My thoughts for fixing this would be either using static_cast<unsigned long long>(-1) or adding typedef unsigned long long ulonglong; to BLI_sys_types.h and then doing ulonglong(-1). Though, depending on feedback from the already requested changes, this code may be replaced with using fixed-with integer types, which would avoid the issue.

I didn't know that functional-style casts can only be done with simple type specifiers, and for some reason it does compile on Windows. My thoughts for fixing this would be either using `static_cast<unsigned long long>(-1)` or adding `typedef unsigned long long ulonglong;` to `BLI_sys_types.h` and then doing `ulonglong(-1)`. Though, depending on feedback from the already requested changes, this code may be replaced with using fixed-with integer types, which would avoid the issue.
Hans Goudey commented 2023-12-21 02:52:01 +01:00
Outdated
Review
Copy Link

Not sure about how it fits with the other Python code, but if possible, fixed width types seem nicest. Other than that static_cast is totally fine here. Appreciate your desire to follow the style guide though :)

Not sure about how it fits with the other Python code, but if possible, fixed width types seem nicest. Other than that `static_cast` is totally fine here. Appreciate your desire to follow the style guide though :)
}
unsigned long long to_return = PyLong_AsUnsignedLongLong(value_converted);
Py_DECREF(value_converted);
return to_return;
}
int PyC_Long_AsBool(PyObject *value)
{
const int test = _PyLong_AsInt(value);

11
source/blender/python/generic/py_capi_utils.h
View File

@ -268,6 +268,17 @@ const char *PyC_StringEnum_FindIDFromValue(const struct PyC_StringEnumItems *ite
*/
int PyC_CheckArgs_DeepCopy(PyObject *args);
/* Unsigned integer parsing that calls `__index__` like signed integer parsing. */
/**
* #PyLong_AsLong and #PyLong_AsLongLong call the Python `__index__` method of the input `value` if
* it is not a `PyLongObject`.
*
* #PyLong_AsUnsignedLong and #PyLong_AsUnsignedLongLong, however, do not call `__index__`, which
* means they cannot parse NumPy scalars and other numeric types.
*/
ulong PyC_Long_AsUnsignedLong(PyObject *value);
unsigned long long PyC_Long_AsUnsignedLongLong(PyObject *value);
Thomas Barlow commented 2023-12-20 18:44:10 +01:00
Outdated
Review
Copy Link

It does look a little odd having unsigned long long next to ulong. I could add typedef unsigned long long ulonglong; to BLI_sys_types.h and then change use of unsigned long long in this PR to ulonglong.

It does look a little odd having `unsigned long long` next to `ulong`. I could add `typedef unsigned long long ulonglong;` to `BLI_sys_types.h` and then change use of `unsigned long long` in this PR to `ulonglong`.
Brecht Van Lommel commented 2023-12-20 20:14:40 +01:00
Outdated
Review
Copy Link

From the Blender point of view we care about these being uint32_t and uint64_t. So I would suggest to use those types here and AsU32 and AsU64 in the function name.

From the Blender point of view we care about these being `uint32_t` and `uint64_t`. So I would suggest to use those types here and `AsU32` and `AsU64` in the function name.
Thomas Barlow commented 2023-12-20 21:19:01 +01:00
Outdated
Review
Copy Link

I didn't think that ulong would be guaranteed to fit in a uint32_t since I thought it was usually 64-bit on 64-bit Linux. Similarly for fitting unsigned long long into uint64_t which apparently is sometimes 128-bit or 96-bit padded to 128-bit. (Edit: I was thinking of long double, I'm not sure in what cases long long could be larger than 64-bit.) Unless Blender is compiled with settings that force these types to be those fixed sizes? I've seen assumptions about the size of int and smaller types in other parts of Blender's source, but not long and long long.

Since the intent is to replace #PyLong_AsUnsignedLong and #PyLong_AsUnsignedLongLong from CPython's API, I thought it would be better to use their return types and keep the names similar. There are also already implementations for #PyC_Long_AsU32 and similar in py_capi_utils.h.

I didn't think that `ulong` would be guaranteed to fit in a `uint32_t` since I thought it was usually 64-bit on 64-bit Linux. ~~Similarly for fitting `unsigned long long` into `uint64_t` which apparently is sometimes 128-bit or 96-bit padded to 128-bit.~~ (Edit: I was thinking of `long double`, I'm not sure in what cases `long long` could be larger than 64-bit.) Unless Blender is compiled with settings that force these types to be those fixed sizes? I've seen assumptions about the size of `int` and smaller types in other parts of Blender's source, but not `long` and `long long`. Since the intent is to replace `#PyLong_AsUnsignedLong` and `#PyLong_AsUnsignedLongLong` from CPython's API, I thought it would be better to use their return types and keep the names similar. There are also already implementations for `#PyC_Long_AsU32` and similar in `py_capi_utils.h`.
Thomas Barlow commented 2023-12-21 12:44:06 +01:00
Outdated
Review
Copy Link

I've had a further think about this and I don't think ulong potentially being 64-bit will be an issue because there is no PROP_RAW_ULONG and code should be using fixed-width types where possible.

I can instead implement the __index__() fallback directly into PyC_Long_AsU64, PyC_Long_AsU32, PyC_Long_AsU16 and PyC_Long_AsU8. I would remove both of the new functions in py_capi_utils.h and refactor PyC_Long_AsUnsignedLong into a static ulong pyc_Long_AsUnsignedLong(PyObject *value) function in py_capi_utils.cc, to avoid duplicate code in the 32-bit and smaller functions.

This would keep py_capi_utils.h using fixed-width types for integer parsing and would encourage the use of one of the fixed-width unsigned integer parsing functions instead of using PyLong_AsUnsignedLong or PyLong_AsUnsignedLongLong.

I've had a further think about this and I don't think `ulong` potentially being 64-bit will be an issue because there is no `PROP_RAW_ULONG` and code should be using fixed-width types where possible. I can instead implement the `__index__()` fallback directly into `PyC_Long_AsU64`, `PyC_Long_AsU32`, `PyC_Long_AsU16` and `PyC_Long_AsU8`. I would remove both of the new functions in `py_capi_utils.h` and refactor `PyC_Long_AsUnsignedLong` into a `static ulong pyc_Long_AsUnsignedLong(PyObject *value)` function in `py_capi_utils.cc`, to avoid duplicate code in the 32-bit and smaller functions. This would keep `py_capi_utils.h` using fixed-width types for integer parsing and would encourage the use of one of the fixed-width unsigned integer parsing functions instead of using `PyLong_AsUnsignedLong` or `PyLong_AsUnsignedLongLong`.
Brecht Van Lommel commented 2023-12-21 12:56:52 +01:00
Outdated
Review
Copy Link

This sounds good to me! In general we want to avoid using long in Blender wherever we can, in the places we do need it having it more contained like this helps.

This sounds good to me! In general we want to avoid using `long` in Blender wherever we can, in the places we do need it having it more contained like this helps.
/* Integer parsing (with overflow checks), -1 on error. */
/**
* Comparison with #PyObject_IsTrue

6
source/blender/python/intern/bpy_rna.cc
View File

@ -5507,13 +5507,13 @@ static PyObject *foreach_getset(BPy_PropertyRNA *self, PyObject *args, int set)
((int8_t *)array)[i] = int8_t(PyLong_AsLong(item));
break;
case PROP_RAW_UINT8:
((uint8_t *)array)[i] = uint8_t(PyLong_AsUnsignedLong(item));
((uint8_t *)array)[i] = uint8_t(PyC_Long_AsUnsignedLong(item));
break;
case PROP_RAW_SHORT:
((short *)array)[i] = short(PyLong_AsLong(item));
break;
case PROP_RAW_UINT16:
((uint16_t *)array)[i] = uint16_t(PyLong_AsUnsignedLong(item));
((uint16_t *)array)[i] = uint16_t(PyC_Long_AsUnsignedLong(item));
break;
case PROP_RAW_INT:
((int *)array)[i] = int(PyLong_AsLong(item));
@ -5531,7 +5531,7 @@ static PyObject *foreach_getset(BPy_PropertyRNA *self, PyObject *args, int set)
((int64_t *)array)[i] = int64_t(PyLong_AsLongLong(item));
break;
case PROP_RAW_UINT64:
((uint64_t *)array)[i] = uint64_t(PyLong_AsUnsignedLongLong(item));
((uint64_t *)array)[i] = uint64_t(PyC_Long_AsUnsignedLongLong(item));
break;
case PROP_RAW_UNSET:
/* Should never happen. */