Gmp warning to error #5882
Gmp warning to error #5882
Conversation
Girgias
force-pushed
the
gmp-warning-to-error
branch
3 times, most recently
from
4c07065 to
a0d7f93
Compare
Girgias
force-pushed
the
gmp-warning-to-error
branch
from
a0d7f93 to
c3789cf
Compare
|
@kocsismate can you have a look at some of the error messages in here? |
Girgias
force-pushed
the
gmp-warning-to-error
branch
from
c3789cf to
e1c3356
Compare
|
Are there other issues/objections to this? Or can I merge this before FF gets tagged on Tuesday? |
Girgias
force-pushed
the
gmp-warning-to-error
branch
from
e1c3356 to
85a4dbe
Compare
ext/gmp/gmp.c
Outdated
| @@ -381,6 +384,10 @@ static int gmp_do_operation_ex(zend_uchar opcode, zval *result, zval *op1, zval | |||
| DO_BINARY_UI_OP_EX(mpz_tdiv_q, gmp_mpz_tdiv_q_ui, 1); | |||
| case ZEND_MOD: | |||
| DO_BINARY_UI_OP_EX(mpz_mod, gmp_mpz_mod_ui, 1); | |||
| if (UNEXPECTED(EG(exception))) { | |||
| return FAILURE; | |||
There was a problem hiding this comment.
Is it actually necessary to return failure on exception here? I think if we throw, this should still be success, otherwise we're going to try calling do_operation on the second operand again.
There was a problem hiding this comment.
I forgot to drop this again but the DO_BINARY_UI_OP_EX macro does exactly that:
#define DO_BINARY_UI_OP_EX(op, uop, check_b_zero) \
gmp_zval_binary_ui_op( \
result, op1, op2, op, uop, check_b_zero \
); \
if (UNEXPECTED(EG(exception))) { return FAILURE; } \
return SUCCESS;
ext/gmp/gmp.c
Outdated
| RETURN_FALSE; | ||
| } | ||
| FETCH_GMP_ZVAL(gmpnum, a_arg, temp_a, 1); | ||
| FREE_GMP_TEMP(temp_a); |
There was a problem hiding this comment.
Shouldn't this function just accept int instead? Makes little sense to convert to GMP first and then convert back to integer.
There was a problem hiding this comment.
Well the function should be able to accept a GMP object, but it's true that this code makes little sense atm.
Girgias
force-pushed
the
gmp-warning-to-error
branch
from
85a4dbe to
b0c77ff
Compare
This gets rids of most false returns.