Skip to content

Explicit octal notation for integers #6360

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 1 commit into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Oct 20, 2020

RFC still needs to be written but the primary motivation is that "016" == 016 is equal to false as 016 is interpreted as an octal integer and can be surprising.

RFC: https://wiki.php.net/rfc/explicit_octal_notation

Albeit PHP's way of dealing with octal is well established (C, Java, Golang, Haskell, etc.) other languages provide the 0o prefix for octal integer analogous to 0b and 0x for binary and hexadecimal integers.

TODO: Add support for:

  • filter_var('0o77', FILTER_VALIDATE_INT, FILTER_FLAG_ALLOW_OCTAL)
  • octdec('0o77') (Actually PHP already supports this)

References:

@Girgias Girgias added the RFC label Oct 20, 2020
@Girgias Girgias self-assigned this Oct 20, 2020
@Girgias Girgias force-pushed the explicit-octal-notation branch from e923320 to b9f9eea Compare October 21, 2020 02:25
@Girgias Girgias force-pushed the explicit-octal-notation branch from b9f9eea to 28d7486 Compare October 27, 2020 03:20
@Girgias Girgias force-pushed the explicit-octal-notation branch from 28d7486 to bf13d7c Compare November 3, 2020 09:46
/* zend_oct_strtod skips leading '0' */
ZVAL_DOUBLE(zendlval, zend_oct_strtod(octal, (const char **)&end));
ZEND_ASSERT(!errno);
/* errno isn't checked since we allow HUGE_VAL/INF overflow */
Copy link
Member

Choose a reason for hiding this comment

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

Mismatch between this comment and the preceding assert. Can you please add a test for a very large octal literal that overflows to INF?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, I'm adding a bunch of tests for the other integer literals while I'm at it.

@Girgias Girgias force-pushed the explicit-octal-notation branch 3 times, most recently from 61b83ec to 4e68814 Compare November 30, 2020 15:24
@Girgias Girgias force-pushed the explicit-octal-notation branch 2 times, most recently from 0b6a8a3 to 7a32bf1 Compare December 21, 2020 21:31
@Girgias Girgias force-pushed the explicit-octal-notation branch from 7a32bf1 to cae80af Compare January 4, 2021 17:34
@Girgias Girgias requested a review from nikic January 4, 2021 17:37
Copy link
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.

Looks fine assuming CI is happy...

strip_underscores(octal, &len);
}

/* Reset errno */
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not needed ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

Will drop

echo 'Octal to decimal:', \PHP_EOL;
var_dump(base_convert('0o', 8, 10));
var_dump(base_convert('0O', 8, 10));
var_dump(base_convert('0', 8, 10)); // This is somehow deprecated????
Copy link
Member

Choose a reason for hiding this comment

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

What's up with this 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.

Ah right, that was when I messed up by confusing 0 and O ... will drop it

@php-pulls php-pulls closed this in 589bdf3 Jan 4, 2021
@Girgias Girgias deleted the explicit-octal-notation branch January 4, 2021 20:10
jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this pull request Feb 28, 2022
This add support for the explicit octal notation using a `0o`/`0O` prefix as supported in PHP as of PHP 8.1.

The only method which needed adjusting was the `Numbers::getCompleteNumber()` method, which now backfills the tokenization when needed.

For the `Numbers::isOctalInt()` method, a small tweak to the regex was all that was needed.

As for the `Numbers::getDecimalValue()` method: this already handled the conversion correctly due to the use of the PHP native `octdec()` function.

From the RFC:
> Surprisingly PHP already has support for this notation when using the `octdec()` and `base_convert()` functions.

Includes adding unit tests to safeguard support in all relevant methods in the class.

Refs:
* https://wiki.php.net/rfc/explicit_octal_notation
* php/php-src#6360
* squizlabs/PHP_CodeSniffer#3481
* squizlabs/PHP_CodeSniffer#3552
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Dec 5, 2022
…niff

> **Integer Octal Literal Prefix**
>
> Octal integers can now use an explicit `0o`/`0O` prefix in integer literals, similarly to binary and hexadecimal integer literals.

Includes unit tests.
Includes documentation.

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.core.octal-literal-prefix
* https://wiki.php.net/rfc/explicit_octal_notation
* php/php-src#6360
* php/php-src@589bdf3
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Dec 5, 2022
…niff

> **Integer Octal Literal Prefix**
>
> Octal integers can now use an explicit `0o`/`0O` prefix in integer literals, similarly to binary and hexadecimal integer literals.

Includes unit tests.
Includes documentation.

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.core.octal-literal-prefix
* https://wiki.php.net/rfc/explicit_octal_notation
* php/php-src#6360
* php/php-src@589bdf3
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