Refactor BCMath bundledlib and extension #10774
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4ff57ba to
63f191b
Compare
jorgsowa
reviewed
Jul 8, 2023
There was a problem hiding this comment.
Hey @Girgias. I saw your PR fixing formatting in this extension and would be awesome to merge it.
Although I'm not very convinced of using size_t. I would leave int, however I'm not still very experienced in PHP internal structure and don't know benefits of this type.
63f191b to
44fe404
Compare
jorgsowa
reviewed
Jul 20, 2023
ext/bcmath/libbcmath/src/compare.c
Outdated
Comment on lines
107
to
131
| /* They are equal up to the last part of the equal part of the fraction. */ | ||
| if (n1->n_scale != n2->n_scale) { | ||
| if (n1->n_scale > n2->n_scale) { | ||
| for (count = n1->n_scale-n2->n_scale; count>0; count--) { | ||
| if (*n1ptr++ != 0) { | ||
| /* Magnitude of n1 > n2. */ | ||
| if (!use_sign || n1->n_sign == PLUS) { | ||
| return (1); | ||
| } else { | ||
| return (-1); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| for (count = n2->n_scale-n1->n_scale; count>0; count--) { | ||
| if (*n2ptr++ != 0) { | ||
| /* Magnitude of n1 < n2. */ | ||
| if (!use_sign || n1->n_sign == PLUS) { | ||
| return (-1); | ||
| } else { | ||
| return (1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hey @Girgias! I analyzed your PR and found the cause of the failing tests. Problem is with if/else conditions here. You changed scope of top level if in those lines. Top level if doesn't have else statement.
If you need help with this PR I offer it, because I would like to have this extension tidied up. 🙌🏾
There was a problem hiding this comment.
Thank you that was very helpful :)
Was busy with trying to get stuff done for feature freeze, so if the rebase, fix + force push has everything working on CI I'll merge this. :)
44fe404 to
7e149b5
Compare
And add braces to block statements, as the current code was pretty much unreadable with how inconsistent it was.
This seperates the concerns of throwing exceptions back into the PHP_FUNCTION instead of being the responsability of the library
This seperates the concerns of throwing exceptions back into the PHP_FUNCTION instead of being the responsability of the library
Return false on division by 0 attempt instead of -1 and true on success instead of 0
Return false on division by 0 attempt instead of -1 and true on success instead of 0
7e149b5 to
01fa580
Compare
jorgsowa
pushed a commit
to jorgsowa/php-src
that referenced
this pull request
Jul 26, 2023
* ext/bcmath: coding style: use indentation And add braces to block statements, as the current code was pretty much unreadable with how inconsistent it was. * ext/bcmath: Remove some useless header inclusions * ext/bcmath: Use standard C99 bool type instead of char * ext/bcmath: Include specific headers instead of config.h * Restructure definitions to reduce header inclusions * Use size_t as a more appropriate type * Remove unused variable full_scale * Refactor bc_raisemod() to get rid of Zend dependencies This separates the concerns of throwing exceptions back into the PHP_FUNCTION instead of being the responsibility of the library * Refactor bc_raise() to get rid of Zend dependencies This separates the concerns of throwing exceptions back into the PHP_FUNCTION instead of being the responsibility of the library * Refactor bc_divmod() and bc_modulo() to return bool Return false on division by 0 attempt instead of -1 and true on success instead of 0 * Refactor bc_divide() to return bool Return false on division by 0 attempt instead of -1 and true on success instead of 0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've already been injecting ourselves into the library, so may as well have it in a reasonable state.
The first commit is about CS and fixing the very inconsistent indentation to make it readable for a human.
The rest of the changes shouldn't be very controversial as they clarify intent, the only one I am somewhat unsure of is the change from
inttosize_tfor thescalearguments, as I couldn't do it forbc_divide()without hitting either a ASAN error or a complete change in behaviour, similar story with_bc_simp_mul()where changing then2lentype from int tosize_tcauses issues.