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
redundant checks in long_add and long_sub #71260
Comments
------------ the proposed changes ------------
------------ diff ------------ ------------ tests ------------ In addition, I ran 'python -m test' (on my 64-bit Windows 10) before and after applying the patch, and got quite the same output. |
Your analysis and patch look good to me. |
Sorry, I didn't check if the change is valid or not, but:
Please keep the check but as an assertion (Put it in the if block). |
Thanks for the reviews! I added an assert in long_add (in long_sub it might be that the result is 0). |
After giving it some more thought (while working on another, somewhat related issue - http://bugs.python.org/issue27145), I realized that that assert in long_add could further verify that the int x_add returned is a multiple-digit int (as x_add had received two multiple-digit ints to begin with). The important thing about this updated assert is that it verifies that x_add didn't return a reference to an element in small_ints (as small ints must be single-digit ints), so negating it in-place is safe. I have updated the assert and added an appropriate comment. The updated diff file is attached. |
And after quadruple checking myself, I found a foolish mistake - in that flow, x_add received at least one multiple-digit int (not necessarily two :(). I fixed that mistake in the comment. The updated diff file is attached. |
I don't think this assert is needed. Nothing bad happens if the asserted condition is false. On other side, additional assert can slow down debug build (that is already slower than release build). |
I agree. This assert only indirectly verifies that something bad doesn't happen. The bad thing that might happen is an in-place negating of an element of small_ints, so the most direct assert should be 'assert(Py_REFCNT(z) == 1);'. What do you think? |
It would be nice. |
All right. The updated diff file is attached. I compiled and ran the tests again. They are quite the same. The test output is attached. |
Maybe add an assert for the second size negation? |
I considered doing that, but I had already opened another issue (http://bugs.python.org/issue27145) in which I had proposed to replace that in-place negate in long_sub with a call to _PyLong_Negate. Anyway, the second assert would be 'assert(Py_SIZE(z) == 0 || Py_REFCNT(z) == 1);', because if someone does (in Python) 'x = (-2 ** PyLong_SHIFT) - (-2 ** PyLong_SHIFT)', x_sub would do 'return (PyLongObject *)PyLong_FromLong(0);'. The updated diff file and the new test output are attached. (No idea why test_netrc failed there. I ran it specifically five times right after that, and it passed all of them. Maybe some race condition? (To run the tests, I do 'python_d.exe -m test -j3'.) Warning -- files was modified by test_netrc
test test_netrc failed -- Traceback (most recent call last):
File "C:\Users\orenmn\cpython\lib\test\test_netrc.py", line 52, in test_password_with_trailing_hash
""", 'pass#')
File "C:\Users\orenmn\cpython\lib\test\test_netrc.py", line 41, in _test_passwords
nrc = self.make_nrc(nrc)
File "C:\Users\orenmn\cpython\lib\test\test_netrc.py", line 13, in make_nrc
with open(temp_filename, mode) as fp:
PermissionError: [Errno 13] Permission denied: '@test_3652_tmp'
minkernel\crts\ucrt\src\appcrt\lowio\write.cpp(49) : Assertion failed: (_osfile(fh) & FOPEN) ) |
LGTM (except a trailing space in a comment). Thank you for your contribution Oren. |
New changeset c21bf38a9d07 by Serhiy Storchaka in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: