Skip to content

fix bad assumption of strncmp return value #3689

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 2 commits into from

Conversation

swalk-cavium
Copy link
Contributor

This corrects the issue described in
https://bugs.php.net/bug.php?id=74082
The test case was making an incorrect assumption about the return
values from strncmp. The standard library implementation only has to guarantee <,=,>
to zero.

@cmb69
Copy link
Member

cmb69 commented Nov 28, 2018

Thanks for the fix! However, I still think that this test makes no sense, since it claims to test binary strings, but it tests binary numbers, i.e. strings consisting of 0s and 1s only, so it misses its goal (but wastes CPU cycles).

@swalk-cavium
Copy link
Contributor Author

@cmb69 - I admit I was more concerned about making the test pass on aarch64 than
validating the test. Removing it would also achieve that result. Do you have a replacement
test to add?

@nikic
Copy link
Member

nikic commented Nov 29, 2018

I believe this one can just be dropped without replacement. We have other strncmp tests, and this one looks completely bogus to me.

Independently of that, I think it would be good to normalize the return value of strncmp and friends to -1,0,1 (rather than only documenting that the specific value is not guaranteed).

@cmb69
Copy link
Member

cmb69 commented Nov 29, 2018

+1 on dropping this test.

± on normalizing the result; I don't mind, but apparently others do.

@swalk-cavium
Copy link
Contributor Author

So, update the PR to remove the test?

The underlying library calls to the string functions have been standardized
for quite a while (C89 and earlier). It's unlikely those will ever change.

@nikic
Copy link
Member

nikic commented Nov 29, 2018

So, update the PR to remove the test?

Yup!

@cmb69
Copy link
Member

cmb69 commented Nov 29, 2018

Thanks! Applied as ea1ea33 (sorry, missed to sqash).

@cmb69 cmb69 closed this Nov 29, 2018
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.

4 participants