Skip to content

Add support for binary, and octal number prefixes for INI settings #9560

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

Closed
wants to merge 5 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Sep 16, 2022

This gives feature parity with INI setting values to use number prefixes in the same way PHP integers can accept them.

Future scope could be to have support for _ as sepators.

However, I'm not exactly sure how to tackle the negative number case. I don't know if it's sensible to move past the negative sign, compute the values as unsigned and then check for overflows before multiplying it by -1.

@arnaud-lb
Copy link
Member

However, I'm not exactly sure how to tackle the negative number case. I don't know if it's sensible to move past the negative sign, compute the values as unsigned and then check for overflows before multiplying it by -1.

This seems to be nearly what strtol() does in the spec:

7.20.1.4
[...]
If the subject sequence has the expected form and the value of base is between 2 and 36, it is used as the base for conversion, ascribing to each letter its value as given above. If the subject sequence begins with a minus sign, the value resulting from the conversion is negated

@Girgias Girgias force-pushed the ini-prefixed-quantities branch from 67a564a to b172d3b Compare September 20, 2022 09:43
@Girgias Girgias changed the base branch from master to PHP-8.2 September 20, 2022 09:45
@Girgias Girgias marked this pull request as ready for review September 20, 2022 09:45
@Girgias Girgias closed this Sep 20, 2022
@Girgias Girgias reopened this Sep 20, 2022
@Girgias
Copy link
Member Author

Girgias commented Sep 20, 2022

Reopenned to trigger CI which didn't want to start because it was targeting the wrong branch

@Girgias Girgias force-pushed the ini-prefixed-quantities branch 2 times, most recently from e2b9149 to 2a4d294 Compare September 27, 2022 14:24
@Girgias Girgias requested a review from arnaud-lb September 27, 2022 14:26

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Girgias Gina Peter Banyard
This drops support for PHP_MIN_VALUE as we now need to manually change the sign of the result.
Therefore the lowest possible value without overflow is PHP_MIN_VALUE+1.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Girgias Gina Peter Banyard

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Girgias Gina Peter Banyard

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Girgias Gina Peter Banyard

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Girgias Gina Peter Banyard
@Girgias Girgias force-pushed the ini-prefixed-quantities branch from 3fa80ff to e14035f Compare September 29, 2022 11:02
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me

Comment on lines +597 to +616
if (digits[0] == '0' && !isdigit(digits[1])) {
/* Value is just 0 */
if ((digits+1) == str_end) {
*errstr = NULL;
return 0;
}

switch (digits[1]) {
/* Multiplier suffixes */
case 'g':
case 'G':
case 'm':
case 'M':
case 'k':
case 'K':
goto evaluation;
case 'x':
case 'X':
base = 16;
break;
Copy link
Member

@arnaud-lb arnaud-lb Sep 30, 2022

Choose a reason for hiding this comment

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

Nit: We could simplify like this:

Suggested change
if (digits[0] == '0' && !isdigit(digits[1])) {
/* Value is just 0 */
if ((digits+1) == str_end) {
*errstr = NULL;
return 0;
}
switch (digits[1]) {
/* Multiplier suffixes */
case 'g':
case 'G':
case 'm':
case 'M':
case 'k':
case 'K':
goto evaluation;
case 'x':
case 'X':
base = 16;
break;
if (digits[0] == '0') {
switch (digits[1]) {
case 'x':
case 'X':
base = 16;
digits += 2;
break;

This allows to remove the special case for 0, and the multiplier suffix cases in the switch.

We can then remove the if (UNEXPECTED(digits == str_end)) { block (we can add a special case for "no digits after base prefix" in the "no valid leading digits" error bellow when base is not 0.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried doing this and if I try to simplify it then I don't think I can have an error for invalid prefixes

@Girgias Girgias closed this in 0f8b9eb Sep 30, 2022
Girgias added a commit that referenced this pull request Sep 30, 2022

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Girgias Gina Peter Banyard
Closes GH-9560
@Girgias Girgias deleted the ini-prefixed-quantities branch September 30, 2022 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants