Skip to content

Commit

Permalink
From documentation, only the sign of returned value is relevant
Browse files Browse the repository at this point in the history 
With recent glibc, memcmp sometime return a negative value instead of -1
  • Loading branch information
@remicollet
remicollet committed Aug 24, 2017
1 parent 4bf1a11 commit b7e96f8
Showing 1 changed file with 8 additions and 8 deletions.
16 changes: 8 additions & 8 deletions ext/standard/tests/strings/substr_compare.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ var_dump(substr_compare("abcde", "df", -2) < 0);
var_dump(substr_compare("abcde", "bc", 1, 2));
var_dump(substr_compare("abcde", "bcg", 1, 2));
var_dump(substr_compare("abcde", "BC", 1, 2, true));
var_dump(substr_compare("abcde", "bc", 1, 3));
var_dump(substr_compare("abcde", "cd", 1, 2));
var_dump(substr_compare("abcde", "bc", 1, 3) > 0);
var_dump(substr_compare("abcde", "cd", 1, 2) < 0);
var_dump(substr_compare("abcde", "abc", 5, 1));
var_dump(substr_compare("abcde", "abcdef", -10, 10));
var_dump(substr_compare("abcde", "abcdef", -10, 10) < 0);
var_dump(substr_compare("abcde", "abc", 0, 0));
var_dump(substr_compare("abcde", -1, 0, NULL, new stdClass));
echo "Test\n";
var_dump(substr_compare("abcde", "abc", 0, -1));
var_dump(substr_compare("abcde", "abc", -1, NULL, -5));
var_dump(substr_compare("abcde", "abc", -1, NULL, -5) > 0);
var_dump(substr_compare("abcde", -1, 0, "str", new stdClass));

echo "Done\n";
Expand All @@ -25,12 +25,12 @@ bool(true)
int(0)
int(0)
int(0)
int(1)
int(-1)
bool(true)
bool(true)

Warning: substr_compare(): The start position cannot exceed initial string length in %s on line %d
bool(false)
int(-1)
bool(true)
int(0)

Warning: substr_compare() expects parameter 5 to be boolean, object given in %s on line %d
Expand All @@ -39,7 +39,7 @@ Test

Warning: substr_compare(): The length must be greater than or equal to zero in %s on line %d
bool(false)
int(4)
bool(true)

Warning: substr_compare() expects parameter 4 to be integer, string given in %s on line %d
bool(false)
Expand Down

5 comments on commit b7e96f8

@nikic
Copy link
Member

@nikic nikic commented on b7e96f8 Aug 24, 2017

Choose a reason for hiding this comment

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

If that's the case, this should be enforced by the function.

@remicollet
Copy link
Contributor Author

@remicollet remicollet commented on b7e96f8 Aug 24, 2017 •
edited

Choose a reason for hiding this comment

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

@nikic I have think a lot about this... but as per doc, only sign is relevant, why should with force -1, and add some additional tests ? bad for perf ?

P.S. more: there is a single failed assertion in all the test suite... (other fixed for consistency)... so only happens in rare run condition...

@nikic
Copy link
Member

@nikic nikic commented on b7e96f8 Aug 24, 2017

Choose a reason for hiding this comment

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

If it's only in the docs, then it's quite likely that someone, somewhere is still going to rely on specific return values. If we cannot guarantee those values, we should prevent it altogether. Additionally using a normalized result allows you to do things like switch() over the return value. (IIRC for <=> we guarantee that the result is normalized.)

@remicollet
Copy link
Contributor Author

@remicollet remicollet commented on b7e96f8 Aug 24, 2017 •
edited

Choose a reason for hiding this comment

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

@nikic value is already not always -1 / 0 / 1

$ php -r 'var_dump(substr_compare("ab", "abcdef", 0));'
int(-4)
$ php -r 'var_dump(substr_compare("abcdef", "ab", 0));'
int(4)

See https://github.com/php/php-src/blob/master/Zend/zend_operators.c#L2643

return (int)(MIN(length, len1) - MIN(length, len2));

@nikic
Copy link
Member

@nikic nikic commented on b7e96f8 Aug 24, 2017

Choose a reason for hiding this comment

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

@remicollet Sorry, probably I wasn't clear on what I meant. I'm suggesting that we should enforce that substr_compare always returns only -1, 0 or 1. I know it's not the case right now.

Please sign in to comment.