ext/gmp: Improve parsing of parameters #16685
Conversation
ext/gmp/tests/gmp_cmp.phpt
Outdated
| @@ -26,7 +26,7 @@ try { | |||
| echo "Done\n"; | |||
| ?> | |||
| --EXPECT-- | |||
| int(2) | |||
| int(1) | |||
There was a problem hiding this comment.
Maybe instead something like this $res = gmp_cmp(123123,-123123); <if 64 bits expects 1 or die>... wdyt ?
| @@ -26,7 +26,7 @@ try { | |||
| echo "Done\n"; | |||
| ?> | |||
| --EXPECT-- | |||
| int(2) | |||
There was a problem hiding this comment.
This seems to cause a test failure on some CI jobs, so something more is going on
There was a problem hiding this comment.
The documentation of GMP says:
Compare op1 and op2. Return a positive value if op1 > op2, zero if op1 = op2, or a negative value if op1 < op2.
So probably we shouldn't be using the output of the values directly?
There was a problem hiding this comment.
Oh I see, reading source codes, seems mpir and gmp differ with mpz_gmp. The former can return the values -1, >= 0, GMP it is only -1, 0, 1 (if this is what you meant by "normalization" in your gmp_cmp commit).
There was a problem hiding this comment.
Good find! I guess PHP should normalize such that the two libs have the same behaviour
Girgias
force-pushed
the
gmp-zpp-custom
branch
2 times, most recently
from
d4dff8a to
dff6bc0
Compare
Girgias
force-pushed
the
gmp-zpp-custom
branch
from
30f5456 to
dc163dd
Compare
ext/gmp/gmp.c
Outdated
| } | ||
| return FAILURE; | ||
| } | ||
| if (!gmp_zend_parse_arg_into_mpz_ex(op2, &gmp_op2, 1, true)) { |
There was a problem hiding this comment.
You pass arg_num==1 here, is that right? shouldn't it be 2?
| typedef void (*gmp_binary_ui_op_t)(mpz_ptr, mpz_srcptr, gmp_ulong); | ||
| typedef void (*gmp_binary_op2_t)(mpz_ptr, mpz_ptr, mpz_srcptr, mpz_srcptr); | ||
| typedef gmp_ulong (*gmp_binary_ui_op2_t)(mpz_ptr, mpz_ptr, mpz_srcptr, gmp_ulong); | ||
| if (!ZEND_ARG_USES_STRICT_TYPES()) { |
There was a problem hiding this comment.
So strict types only apply on function call arguments right and operators operands should always coerce IIRC. But this code doesn't do that or what am I missing? I think there was a discussion about this at one point.
Girgias
force-pushed
the
gmp-zpp-custom
branch
from
dc163dd to
9fd19a9
Compare
Girgias
force-pushed
the
gmp-zpp-custom
branch
from
65b7a24 to
c924641
Compare
That use being gmp_popcount()
We define module globals to be used with ZPP, which also avoids excessive initializing and clearing of variables, something recommended by the GMP documentation.
It seems that this also now normalizes the return value
Not sure it is the best approach to do this one however
This removes the last usages of the old GMP_FETCH API
Girgias
force-pushed
the
gmp-zpp-custom
branch
from
c924641 to
1b89878
Compare
This PR should be reviewed commit by commit to actually understand what is going on.
The basic idea is to create, and use, a custom Fast ZPP specifier to parse arguments into an
mpz_ptrstructure. To prevent issues with cleaning up, and improve efficiency as recommended by the GMP documentation, we create some module globalmpz_ptrthat we initialize and clean once that we can use during ZPP, without needing to worry about clean-up.This new model greatly simplifies the extension and how to handle parameters, as they will always be
mpz_ptrstructures.The first couple of commits are some fixes and preliminary refactorings, the main one being to convert all relevant calls to Fast ZPP.
I have not benchmarked this, but I would not be surprised if this improves performance of the extension.
This should allow some easier refactoring of the operator overloading handlers to behave more sensibly and not just throw
Errors all the time.