-
Notifications
You must be signed in to change notification settings - Fork 8k
Bug #80545: Converted some type warnings to type errors in the "bcmath" extension. #6545
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
|
That is analogous to what GMP does, but in my opinion, it would be beneficial to have a dedicated function to verify the validity of a number given as string. Also, I think a ValueError would be more appropriate than a TypeError. |
|
@cmb69 I will change it to a value error, that indeed looks more appropriate. There's a function called php_str2num in the bcmath.c file, or would you like to have a globally accessible function? |
I'd rather be consistent with GMP. @Girgias, since you promoted to TypeError, what do you think? /cc @nikic
Yes, this was about a function which could be called from userland. And that function should just return true or false, similar to |
|
I think a ValueError would be better, not exactly sure why I did a TypeError for GMP, but I need to possibly tweak some things with GMP anyway. Would it be possible to use the version with the argument number (IIRC |
That would require to pass the arg num to |
Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
|
Not sure why the CI failed. |
|
I think that CI failure is unrelated to this PR. |
Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
|
@nikic Thanks, I changed the return type from |
Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
nikic
left a comment
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 looks good to me, I'd only suggest to adjust the message to fix the "argument_value_error" format better.
Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
|
@nikic I've pointed this PR to the PHP-8.0 branch, is that correct? |
|
I merged this as 94a151a a while ago, but messed up the auto-close tag. |
Hi,
This my first attempt to solve one of the bugs from the PHP bug tracker (#80545). I targeted this to the PHP 8.0 branch because the warning promotion didn't take place yet in PHP 7.4 (please correct me if I'm wrong).
Bccomp had the same issue, so I changed it there too.
If there's anything I did wrong or needs to change, please let me know! :)