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

Force $binaryPrefixes array ordering on 32-bit systems, fixes #83 #87

Merged
merged 1 commit into from
Oct 12, 2016
Merged

Conversation

Forst
Copy link
Contributor

@Forst Forst commented Oct 12, 2016

This fixes an issue when a 32-bit system breaks associative array ordering when dropping keys over 2^32-1 in String\BinarySuffix and String\MetricSuffix

As noted by @mikaelcom, the following array:

private $binaryPrefixes = array(
    1125899906842624 => '#.## PB',
    1099511627776 => '#.## TB',
    1073741824 => '#.## GB',
    1048576 => '#.## MB',
    1024 => '#.# kB',
    0 => '# bytes',
);

in String\BinarySuffix becomes:

array(4) {
    [0] =>
    string(7) "# bytes"
    [1073741824] =>
    string(7) "#.## GB"
    [1048576] =>
    string(7) "#.## MB"
    [1024] =>
    string(6) "#.# kB"
}

during runtime on 32-bit systems. This breaks the conversion logic, causing a Division by zero exception.

The introduced change sorts the associative array with krsort() so that it gets ordered properly:

array(4) {
    [1073741824] =>
    string(7) "#.## GB"
    [1048576] =>
    string(7) "#.## MB"
    [1024] =>
    string(6) "#.# kB"
    [0] =>
    string(7) "# bytes"
}

Tested on: Ubuntu 14.04.5, kernel 4.4.0-42-generic i686, PHP 5.5.9-1ubuntu4.20.

Should be tested on other architectures and with other values, but I don't think there are any negative consequences to this change.

Copy link
Member

@norberttech norberttech left a comment

Choose a reason for hiding this comment

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

Awesome 👍 🍻

@norberttech norberttech merged commit c47dd0d into coduo:master Oct 12, 2016
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