Skip to content
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

BigNumber::of performance update #77

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

SebastienDug
Copy link
Contributor

  1. Split regex for Rational and Numerical
  2. Exception method is static to the class instead of being created dynamically for each call to BigNumber::of
  3. Added flag to pregMatch to return no matches as NULL instead of manipulating empty string and converting it to null
  4. leanCleanup introduced as there was a concatenation of sign to pass to the cleanup function which then removes the sign from the string

Copy link
Member

@BenMorel BenMorel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR, this brings an appreciable performance improvement!

Let's improve a few things and merge it!

src/BigNumber.php Outdated Show resolved Hide resolved
src/BigNumber.php Outdated Show resolved Hide resolved

$unscaledValue = self::cleanUp(($sign ?? ''). $integral . $fractional);
$unscaledValue = self::leanCleanUp(($sign ?? ''), $integral . $fractional);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$unscaledValue = self::leanCleanUp(($sign ?? ''), $integral . $fractional);
$unscaledValue = self::cleanUp($sign, $integral . $fractional);

After leanCleanUp has been renamed to cleanUp.

Comment on lines 25 to 28
'(?<integral>[0-9]+)?' .
'(?<point>\.)?' .
'(?<fractional>[0-9]+)?' .
'(?:[eE](?<exponent>[\-\+]?[0-9]+))?' .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]+)' .
'$/';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/^' .
'(?<sign>[\-\+])?' .
'(?<numerator>[0-9]+)' .
'\/?' .
'(?<denominator>[0-9]+)' .
'$/';
'/^' .
'(?<sign>[\-\+])?' .
'(?<numerator>[0-9]+)' .
'\/?' .
'(?<denominator>[0-9]+)' .
'$/';

Comment on lines 149 to 153
/**
* Throws a NumberFormatException for the given value.
* @psalm-pure
*/
protected static function throwException(string $value) : void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

}

return new BigDecimal($unscaledValue, $scale);
}
$integral = self::leanCleanUp(($sign ?? ''), $integral);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$integral = self::leanCleanUp(($sign ?? ''), $integral);
$integral = self::cleanUp($sign, $integral);

After leanCleanUp has been renamed to cleanUp.

@SebastienDug
Copy link
Contributor Author

SebastienDug commented Nov 27, 2023 via email

@BenMorel BenMorel merged commit 57a1758 into brick:master Nov 29, 2023
14 checks passed
@BenMorel
Copy link
Member

Thank you, @SebastienDug!

@BenMorel
Copy link
Member

Released as 0.12.1.

@SebastienDug
Copy link
Contributor Author

SebastienDug commented Nov 30, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants