-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Vector{2,3}.__delattr__
messaging fixed when deleting x, y, {z}
and removed Vector4
zombies
#3069
Conversation
…and removed Vector4 zombie remnants
src_c/math.c
Outdated
break; | ||
} | ||
case 2: { | ||
if (self->dim >= 3) { |
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.
Why is this check needed? I think in this path an error should be raised either way regardless of dim value
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.
Differentiate between vector2 and vector3. This also guarantees the old behavior of going to badinternalcall if somehow someone manages to get a vec2 with a z property
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 old code checked for
if (component >= self->dim) {
PyErr_BadInternalCall();
}
Irrespective of the status of value
but the changes do not preserve this behaviour.
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.
removed the check
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.
Your latest commit didn't address what I was trying to say.
Consider a call where value != NULL && component >= self->dim
. In this case the old code correctly raises an error but the new code doesn't. The PyErr_BadInternalCall
check should happen independently of the NULL
check outside the block and above it.
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.
Looks good to me, thanks!
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.
OK, LGTM. The code makes sense and I can verify that the new tests fail before this PR is compiled and pass afterwards (tested locally).
Fixes #3066 and removes the remnants of whatever Vector4 used to be there