Skip to content

Refactor BCMath 1 #14076

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Refactor BCMath 1 #14076

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1d2924
Reorder structure members to account for alignment
SakiTakamachi Apr 29, 2024
4d6e4b1
refactor bc_str2num
SakiTakamachi May 1, 2024
7212ba3
fixed comment
SakiTakamachi May 1, 2024
386cb7f
address comments
SakiTakamachi May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions ext/bcmath/bcmath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,7 @@ PHP_MINFO_FUNCTION(bcmath)
Convert to bc_num detecting scale */
static zend_result php_str2num(bc_num *num, char *str)
{
char *p;

if (!(p = strchr(str, '.'))) {
if (!bc_str2num(num, str, 0)) {
return FAILURE;
}

return SUCCESS;
}

if (!bc_str2num(num, str, strlen(p+1))) {
if (!bc_str2num(num, str, 0, true)) {
return FAILURE;
}

Expand Down Expand Up @@ -624,12 +614,12 @@ PHP_FUNCTION(bccomp)
bc_init_num(&first);
bc_init_num(&second);

if (!bc_str2num(&first, ZSTR_VAL(left), scale)) {
if (!bc_str2num(&first, ZSTR_VAL(left), scale, false)) {
zend_argument_value_error(1, "is not well-formed");
goto cleanup;
}

if (!bc_str2num(&second, ZSTR_VAL(right), scale)) {
if (!bc_str2num(&second, ZSTR_VAL(right), scale, false)) {
zend_argument_value_error(2, "is not well-formed");
goto cleanup;
}
Expand Down
6 changes: 3 additions & 3 deletions ext/bcmath/libbcmath/src/bcmath.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ typedef enum {PLUS, MINUS} sign;
typedef struct bc_struct *bc_num;

typedef struct bc_struct {
sign n_sign;
size_t n_len; /* The number of digits before the decimal point. */
size_t n_scale; /* The number of digits after the decimal point. */
int n_refs; /* The number of pointers to this number. */
char *n_ptr; /* The pointer to the actual storage.
If NULL, n_value points to the inside of another number
(bc_multiply...) and should not be "freed." */
char *n_value; /* The number. Not zero char terminated.
May not point to the same place as n_ptr as
in the case of leading zeros generated. */
int n_refs; /* The number of pointers to this number. */
sign n_sign;
} bc_struct;

#ifdef HAVE_CONFIG_H
Expand Down Expand Up @@ -97,7 +97,7 @@ bc_num bc_copy_num(bc_num num);

void bc_init_num(bc_num *num);

bool bc_str2num(bc_num *num, char *str, size_t scale);
bool bc_str2num(bc_num *num, char *str, size_t scale, bool auto_scale);

zend_string *bc_num2str_ex(bc_num num, size_t scale);

Expand Down
127 changes: 72 additions & 55 deletions ext/bcmath/libbcmath/src/str2num.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,23 @@

/* Convert strings to bc numbers. Base 10 only.*/

bool bc_str2num(bc_num *num, char *str, size_t scale)
bool bc_str2num(bc_num *num, char *str, size_t scale, bool auto_scale)
{
size_t digits = 0;
size_t strscale = 0;
char *ptr, *nptr;
size_t trailing_zeros = 0;
size_t str_scale = 0;
char *ptr = str;
char *nptr;
char *integer_ptr;
char *integer_end;
char *fractional_ptr = NULL;
char *fractional_end = NULL;
char *decimal_point;
bool zero_int = false;

/* Prepare num. */
bc_free_num (num);

/* Check for valid number and count digits. */
ptr = str;

if ((*ptr == '+') || (*ptr == '-')) {
/* Skip Sign */
ptr++;
Expand All @@ -57,77 +60,91 @@ bool bc_str2num(bc_num *num, char *str, size_t scale)
while (*ptr == '0') {
ptr++;
}
integer_ptr = ptr;
Copy link
Member

Choose a reason for hiding this comment

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

I'll only put this comment once as it applies to some other places too.
When refactoring code, you should merge the declaration and the assignment where possible.
I.e. make this char *integer_ptr = ptr;. Or even const char *integer_ptr = ptr;.
The reason to merge them is so it becomes clearer what the actual scope of the variable is, so it's not accidentally used when it should not be used (yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 386cb7f

/* digits before the decimal point */
while (*ptr >= '0' && *ptr <= '9') {
ptr++;
digits++;
}
/* decimal point */
if (*ptr == '.') {
ptr++;
decimal_point = (*ptr == '.') ? ptr : NULL;

/* If a string other than numbers exists */
if (!decimal_point && *ptr != '\0') {
goto fail;
}
/* digits after the decimal point */
while (*ptr >= '0' && *ptr <= '9') {
if (*ptr == '0') {
trailing_zeros++;
} else {
trailing_zeros = 0;

/* search and validate fractional end if exists */
if (decimal_point) {
/* search */
fractional_ptr = fractional_end = decimal_point + 1;
if (*fractional_ptr == '\0') {
goto after_fractional;
}
ptr++;
strscale++;
}

if (trailing_zeros > 0) {
/* Trailing zeros should not take part in the computation of the overall scale, as it is pointless. */
strscale = strscale - trailing_zeros;
/* validate */
while (*fractional_ptr >= '0' && *fractional_ptr <= '9') {
fractional_end = *fractional_ptr != '0' ? fractional_ptr + 1 : fractional_end;
fractional_ptr++;
}
if (*fractional_ptr != '\0') {
/* invalid num */
goto fail;
}
fractional_ptr = decimal_point + 1;

str_scale = fractional_end - fractional_ptr;
Copy link
Member

Choose a reason for hiding this comment

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

This may need a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 386cb7f

if (str_scale > scale && !auto_scale) {
fractional_end -= str_scale - scale;
str_scale = scale;
}
}
if ((*ptr != '\0') || (digits + strscale == 0)) {
*num = bc_copy_num(BCG(_zero_));
return *ptr == '\0';

after_fractional:

if (digits + str_scale == 0) {
goto zero;
}

/* Adjust numbers and allocate storage and initialize fields. */
strscale = MIN(strscale, scale);
if (digits == 0) {
zero_int = true;
digits = 1;
}
*num = bc_new_num (digits, strscale);

/* Build the whole number. */
ptr = str;
if (*ptr == '-') {
(*num)->n_sign = MINUS;
ptr++;
} else {
(*num)->n_sign = PLUS;
if (*ptr == '+') ptr++;
}
/* Skip leading zeros. */
while (*ptr == '0') {
ptr++;
}
*num = bc_new_num (digits, str_scale);
(*num)->n_sign = *str == '-' ? MINUS : PLUS;
nptr = (*num)->n_value;
if (zero_int) {
*nptr++ = 0;
digits = 0;
}
for (; digits > 0; digits--) {
*nptr++ = CH_VAL(*ptr++);
}

/* Build the fractional part. */
if (strscale > 0) {
/* skip the decimal point! */
ptr++;
for (; strscale > 0; strscale--) {
*nptr++ = CH_VAL(*ptr++);
if (zero_int) {
nptr++;
while (fractional_ptr < fractional_end) {
*nptr = CH_VAL(*fractional_ptr);
nptr++;
fractional_ptr++;
}
} else {
integer_end = integer_ptr + digits;
while (integer_ptr < integer_end) {
*nptr = CH_VAL(*integer_ptr);
nptr++;
integer_ptr++;
}
if (str_scale > 0) {
while (fractional_ptr < fractional_end) {
*nptr = CH_VAL(*fractional_ptr);
nptr++;
fractional_ptr++;
}
}
}

if (bc_is_zero(*num)) {
(*num)->n_sign = PLUS;
}
return true;

zero:
*num = bc_copy_num(BCG(_zero_));
return true;

fail:
*num = bc_copy_num(BCG(_zero_));
return false;
}
Loading