-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/bcmath: null
should not be supported for operator overloading + comparison issues
#15875
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
Conversation
@Girgias |
I forgot to do this on failure.
|
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.
This is fine, I have one minor remark/nit
ext/bcmath/bcmath.c
Outdated
@@ -1280,8 +1283,15 @@ static int bcmath_number_compare(zval *op1, zval *op2) | |||
|
|||
return (int) ret; | |||
|
|||
fallback: | |||
return zend_std_compare_objects(op1, op2); | |||
failure: |
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.
If you declare int ret = ZEND_UNCOMPARABLE;
at the start of the function, then you can put the failure label at line 1276, which would reduce code duplication.
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.
Done, sorry for the delay
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.
LGTM, thanks!
edit:
Please correct only the points mentioned by Niels :)
CI is failing because of cURL tests and a failure to set up MySQL |
da4d6af
to
373c7d1
Compare
@SakiTakamachi you have memory leaks with the handling of the comparison operators and some conversions to string which don't make much sense to me (namely
array
to string).