BigNumber::of performance update #77
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
Contributor
SebastienDug
commented
Nov 10, 2023
- Split regex for Rational and Numerical
- Exception method is static to the class instead of being created dynamically for each call to BigNumber::of
- Added flag to pregMatch to return no matches as NULL instead of manipulating empty string and converting it to null
- leanCleanup introduced as there was a concatenation of sign to pass to the cleanup function which then removes the sign from the string
BenMorel
requested changes
Nov 18, 2023
Member
BenMorel
left a comment
There was a problem hiding this comment.
Thank you for your PR, this brings an appreciable performance improvement!
Let's improve a few things and merge it!
src/BigNumber.php
Outdated
| } | ||
|
|
||
| $unscaledValue = self::cleanUp(($sign ?? ''). $integral . $fractional); | ||
| $unscaledValue = self::leanCleanUp(($sign ?? ''), $integral . $fractional); |
Member
There was a problem hiding this comment.
Suggested change
| $unscaledValue = self::leanCleanUp(($sign ?? ''), $integral . $fractional); | |
| $unscaledValue = self::cleanUp($sign, $integral . $fractional); |
After leanCleanUp has been renamed to cleanUp.
src/BigNumber.php
Outdated
Comment on lines
25
to
28
| '(?<integral>[0-9]+)?' . | ||
| '(?<point>\.)?' . | ||
| '(?<fractional>[0-9]+)?' . | ||
| '(?:[eE](?<exponent>[\-\+]?[0-9]+))?' . |
Member
There was a problem hiding this comment.
Suggested change
| '(?<integral>[0-9]+)?' . | |
| '(?<point>\.)?' . | |
| '(?<fractional>[0-9]+)?' . | |
| '(?:[eE](?<exponent>[\-\+]?[0-9]+))?' . | |
| '(?<integral>[0-9]+)?' . | |
| '(?<point>\.)?' . | |
| '(?<fractional>[0-9]+)?' . | |
| '(?:[eE](?<exponent>[\-\+]?[0-9]+))?' . |
Comment on lines
35
to
40
| '/^' . | ||
| '(?<sign>[\-\+])?' . | ||
| '(?<numerator>[0-9]+)' . | ||
| '\/?' . | ||
| '(?<denominator>[0-9]+)' . | ||
| '$/'; |
Member
There was a problem hiding this comment.
Suggested change
| '/^' . | |
| '(?<sign>[\-\+])?' . | |
| '(?<numerator>[0-9]+)' . | |
| '\/?' . | |
| '(?<denominator>[0-9]+)' . | |
| '$/'; | |
| '/^' . | |
| '(?<sign>[\-\+])?' . | |
| '(?<numerator>[0-9]+)' . | |
| '\/?' . | |
| '(?<denominator>[0-9]+)' . | |
| '$/'; |
src/BigNumber.php
Outdated
Comment on lines
149
to
153
| /** | ||
| * Throws a NumberFormatException for the given value. | ||
| * @psalm-pure | ||
| */ | ||
| protected static function throwException(string $value) : void { |
Member
There was a problem hiding this comment.
Spaces + private:
Suggested change
| /** | |
| * Throws a NumberFormatException for the given value. | |
| * @psalm-pure | |
| */ | |
| protected static function throwException(string $value) : void { | |
| /** | |
| * Throws a NumberFormatException for the given value. | |
| * | |
| * @psalm-pure | |
| */ | |
| private static function throwException(string $value) : void { |
src/BigNumber.php
Outdated
|
|
||
| return new BigDecimal($unscaledValue, $scale); | ||
| } | ||
| $integral = self::leanCleanUp(($sign ?? ''), $integral); |
Member
There was a problem hiding this comment.
Suggested change
| $integral = self::leanCleanUp(($sign ?? ''), $integral); | |
| $integral = self::cleanUp($sign, $integral); |
After leanCleanUp has been renamed to cleanUp.
Contributor
Author
|
Hello again,
Sorry for the delay,
The changes requested have been applied and the branch has been synched.
Thanks,
Sebastien Duguay
|
Member
|
Thank you, @SebastienDug! |
Member
|
Released as 0.12.1. |
Contributor
Author
|
Good to hear.
I will reach out if we find other issues we could improve upon.
Thanks,
Sebastien Duguay
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.