Skip to content

Base Convert changes for PHP 7.4#4328

Closed
exussum12 wants to merge 5 commits intophp:PHP-7.4from
exussum12:php7.4BaseConvert
Closed

Base Convert changes for PHP 7.4#4328
exussum12 wants to merge 5 commits intophp:PHP-7.4from
exussum12:php7.4BaseConvert

Conversation

@exussum12
Copy link
Copy Markdown
Contributor

See https://wiki.php.net/rfc/base_convert_improvements

Old tests are updated for the deprecated error.
These will have to be removed for PHP8

ext/standard/tests/math/base_convert_improvements.phpt contains the new
tests

This takes over from the original PR and contains only the deprecated chars error

See https://wiki.php.net/rfc/base_convert_improvements

Old tests are updated for the depricated error.
These will have to be removed for PHP8

ext/standard/tests/math/base_convert_improvements.phpt contains the new
tests
@exussum12 exussum12 changed the base branch from master to PHP-7.4 June 28, 2019 20:53
Copy link
Copy Markdown
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

I left some implementation notes. This is the overall structure I'd like to see:

char *s = Z_STRVAL_P(arg);
char *e = s + Z_STRLEN_P(arg);

/* Skip leading whitespace */
while (s < e && isspace(*s)) s++;
/* Skip trailing whitespace */
while (s < e && isspace(*(e-1)) e--;

/* Skip 0x, 0o, 0b prefixes */
if (e - s >= 2) {
    if (base == 16 && s[0] == '0' && (s[1] == 'x' || s[1] == 'X')) s += 2;
    // etc
}

/* Main loop */
for (; s < e; s++) {
    // etc
}

@exussum12
Copy link
Copy Markdown
Contributor Author

@nikic added the changes suggested. tests passng and added a test case for spaces in the middle with spaces. Not sure if its ok style wise but changed the empty for loop to a while.

Copy link
Copy Markdown
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! The implementation side looks good to me now.

There are still some test failures on 32-bit systems: https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=893&view=ms.vss-test-web.build-test-results-tab (These tests don't run on 64-bit because the maximum integer size affects the output.)

@exussum12
Copy link
Copy Markdown
Contributor Author

Is there an easy way to bless tests for a 32 bit platform ? Assume I will need a 32 bit Vm to do this ? Its easier to bless tests and then check the diff

@exussum12
Copy link
Copy Markdown
Contributor Author

@nikic / @derickr anything else I need to do to this PR ? Got a second PR for master (PHP 8) but that is a fairly big test change

@nikic
Copy link
Copy Markdown
Member

nikic commented Jul 8, 2019

Merged as d90cdbd. Hopefully I got the merge to master right.

Thanks for working on this!

@nikic nikic closed this Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants