Fix #3812 and fix const of inplace assignments #4065
Conversation
…ion007/fix-inplace-operators
|
Ahh, this is the operator on pyobject, not |
|
@henryiii Not sure this would actually work because we need to do the same thing on accessors as well. I was thinking about whether it would be better to do a general mixin class like we do the object_api. Thoughts? |
There was a problem hiding this comment.
Very minor comments, the meat of this looks good to me, but can you please give me a chance to run this through global testing? (Results late tonight or tomorrow morning.)
At the moment I'm thinking the const versions are an accident that just happened to survive amazingly long, and that we don't need to bring them back.
include/pybind11/pytypes.h
Outdated
| @@ -353,6 +353,20 @@ class object : public handle { | |||
| return *this; | |||
| } | |||
|
|
|||
| #define PYBIND11_INPLACE_OP(inp) \ | |||
There was a problem hiding this comment.
Very minor technically, but I'd definitely make this iop (a lot more intuitive).
There was a problem hiding this comment.
I actually think this is insufficient for attributes. Basically, we need to define the augmented operators for every object_api have custom operator=.
There was a problem hiding this comment.
Did you count already how many we have?
I'm not sure about the big picture TBH (I may still be misunderstanding), but this PR seemed like a necessary good step in the right direction, just by itself. Does that make sense?
include/pybind11/pytypes.h
Outdated
| @@ -2364,26 +2378,35 @@ bool object_api<D>::rich_compare(object_api const &other, int value) const { | |||
| return result; \ | |||
| } | |||
|
|
|||
| #define PYBIND11_MATH_OPERATOR_BINARY_INPLACE(op, fn) \ | |||
There was a problem hiding this comment.
I'd change this to iop, too (to be consistent & intuitive).
|
I still don't have global testing results unfortunately, due to some internal outage. |
|
Global testing passes for this PR (internal test ID OCL:461063257:BASE:461249872:1657923213067:e8a3caed). Therefore I'm still thinking: no need to worry about @Skylion007 Are there reasons for holding back this PR? (see my previous question) |
|
@rwgk Main reason is I am trying to figure if there is a better way to solve this. Using the augmented assigned operators on handles is still broken with this PR and likely broken for accessors as well. |
|
Okay, so I tested it and the augmented assignment operators for accessors are currently broken anyway due to another bug which results in a compile error (unless you are doing augmented assignments from another accessor). As such, it's not as big of a deal as I thought. I would like to actually remove the augmented assignment operators from handles if possible, but that requires further re-architecting. |
|
Looks like flakes I've seen before. I clicked the button to rerun only the failed job. Let's see what happens. |
Description
So I looked through the inplace assignments and found that many of them did not work for immutable types (int_, str, etc...). I actually couldn't find any situation where they appeared to work properly at all. Additionally, it didn't seem possible to actually properly +=, -= etc in the object_api without some virtual functionsince the = operator is overridden in the subclasses. The methods I suppose could remain const for handles since they don't modify the handle, but we need to make those const methods can't be accidentally used on the object class (perhaps through explicit deletion?).
Fix #3812.
Suggested changelog entry: