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
bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652
Changes from 1 commit
326bd04
6c28b76
5524e62
b6bf9c3
f3603dd
b768077
052fc77
c8219f9
6d56a37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -526,20 +526,17 @@ def test_with_int(self): | |
self.assertEqual(int(id), 10) | ||
|
||
def test_coerce_id(self): | ||
id = interpreters.InterpreterID('10', force=True) | ||
self.assertEqual(int(id), 10) | ||
|
||
id = interpreters.InterpreterID(10.0, force=True) | ||
self.assertEqual(int(id), 10) | ||
|
||
class Int(str): | ||
def __init__(self, value): | ||
self._value = value | ||
def __int__(self): | ||
return self._value | ||
|
||
id = interpreters.InterpreterID(Int(10), force=True) | ||
self.assertEqual(int(id), 10) | ||
for id in ('10', b'10', bytearray(b'10'), memoryview(b'10'), '1_0', | ||
10.0, 10.1, Int(10)): | ||
with self.subTest(id=id): | ||
id = interpreters.InterpreterID(id, force=True) | ||
self.assertEqual(int(id), 10) | ||
|
||
def test_bad_id(self): | ||
for id in [-1, 'spam']: | ||
|
@@ -548,6 +545,8 @@ def test_bad_id(self): | |
interpreters.InterpreterID(id) | ||
with self.assertRaises(OverflowError): | ||
interpreters.InterpreterID(2**64) | ||
with self.assertRaises(OverflowError): | ||
interpreters.InterpreterID(float('inf')) | ||
with self.assertRaises(TypeError): | ||
interpreters.InterpreterID(object()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to have a test still for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I replaced it with |
||
|
||
|
@@ -572,6 +571,14 @@ def test_equality(self): | |
self.assertTrue(id1 == id1) | ||
self.assertTrue(id1 == id2) | ||
self.assertTrue(id1 == int(id1)) | ||
self.assertTrue(int(id1) == id1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice :) |
||
self.assertTrue(id1 == float(int(id1))) | ||
self.assertTrue(float(int(id1)) == id1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one test crashed with the past implementation. |
||
self.assertFalse(id1 == float(int(id1)) + 0.1) | ||
self.assertFalse(id1 == str(int(id1))) | ||
self.assertFalse(id1 == 2**1000) | ||
self.assertFalse(id1 == float('inf')) | ||
self.assertFalse(id1 == 'spam') | ||
self.assertFalse(id1 == id3) | ||
|
||
self.assertFalse(id1 != id1) | ||
|
@@ -1105,20 +1112,17 @@ def test_with_kwargs(self): | |
self.assertEqual(cid.end, 'both') | ||
|
||
def test_coerce_id(self): | ||
cid = interpreters._channel_id('10', force=True) | ||
self.assertEqual(int(cid), 10) | ||
|
||
cid = interpreters._channel_id(10.0, force=True) | ||
self.assertEqual(int(cid), 10) | ||
|
||
class Int(str): | ||
def __init__(self, value): | ||
self._value = value | ||
def __int__(self): | ||
return self._value | ||
|
||
cid = interpreters._channel_id(Int(10), force=True) | ||
self.assertEqual(int(cid), 10) | ||
for id in ('10', b'10', bytearray(b'10'), memoryview(b'10'), '1_0', | ||
10.0, 10.1, Int(10)): | ||
with self.subTest(id=id): | ||
cid = interpreters._channel_id(id, force=True) | ||
self.assertEqual(int(cid), 10) | ||
|
||
def test_bad_id(self): | ||
for cid in [-1, 'spam']: | ||
|
@@ -1164,6 +1168,14 @@ def test_equality(self): | |
self.assertTrue(cid1 == cid1) | ||
self.assertTrue(cid1 == cid2) | ||
self.assertTrue(cid1 == int(cid1)) | ||
self.assertTrue(int(cid1) == cid1) | ||
self.assertTrue(cid1 == float(int(cid1))) | ||
self.assertTrue(float(int(cid1)) == cid1) | ||
self.assertFalse(cid1 == float(int(cid1)) + 0.1) | ||
self.assertFalse(cid1 == str(int(cid1))) | ||
self.assertFalse(cid1 == 2**1000) | ||
self.assertFalse(cid1 == float('inf')) | ||
self.assertFalse(cid1 == 'spam') | ||
self.assertFalse(cid1 == cid3) | ||
|
||
self.assertFalse(cid1 != cid1) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fixed comparing and creating of InterpreterID and ChannelID. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1592,30 +1592,28 @@ channelid_richcompare(PyObject *self, PyObject *other, int op) | |
int equal; | ||
if (PyObject_TypeCheck(other, &ChannelIDtype)) { | ||
channelid *othercid = (channelid *)other; | ||
if (cid->end != othercid->end) { | ||
equal = 0; | ||
} | ||
else { | ||
equal = (cid->id == othercid->id); | ||
} | ||
equal = (cid->end == othercid->end) && (cid->id == othercid->id); | ||
} | ||
else { | ||
other = PyNumber_Long(other); | ||
if (other == NULL) { | ||
PyErr_Clear(); | ||
Py_RETURN_NOTIMPLEMENTED; | ||
} | ||
int64_t othercid = PyLong_AsLongLong(other); | ||
Py_DECREF(other); | ||
if (othercid == -1 && PyErr_Occurred() != NULL) { | ||
else if (PyLong_Check(other)) { | ||
/* Fast path */ | ||
int overflow; | ||
long long othercid = PyLong_AsLongLongAndOverflow(other, &overflow); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use int64_t or the macro? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using But using |
||
if (othercid == -1 && PyErr_Occurred()) { | ||
return NULL; | ||
} | ||
if (othercid < 0) { | ||
equal = 0; | ||
} | ||
else { | ||
equal = (cid->id == othercid); | ||
equal = !overflow && (othercid >= 0) && (cid->id == othercid); | ||
} | ||
else if (PyNumber_Check(other)) { | ||
PyObject *pyid = PyLong_FromLongLong(cid->id); | ||
if (pyid == NULL) { | ||
return NULL; | ||
} | ||
PyObject *res = PyObject_RichCompare(pyid, other, op); | ||
Py_DECREF(pyid); | ||
return res; | ||
} | ||
else { | ||
Py_RETURN_NOTIMPLEMENTED; | ||
} | ||
|
||
if ((op == Py_EQ && equal) || (op == Py_NE && !equal)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,21 +12,17 @@ _Py_CoerceID(PyObject *orig) | |
if (pyid == NULL) { | ||
if (PyErr_ExceptionMatches(PyExc_TypeError)) { | ||
PyErr_Format(PyExc_TypeError, | ||
"'id' must be a non-negative int, got %R", orig); | ||
"'id' must be a non-negative int, got %s", orig->ob_type->tp_name); | ||
} | ||
else { | ||
else if (PyErr_ExceptionMatches(PyExc_ValueError)) { | ||
PyErr_Format(PyExc_ValueError, | ||
"'id' must be a non-negative int, got %R", orig); | ||
} | ||
return -1; | ||
} | ||
int64_t id = PyLong_AsLongLong(pyid); | ||
Py_DECREF(pyid); | ||
if (id == -1 && PyErr_Occurred() != NULL) { | ||
if (!PyErr_ExceptionMatches(PyExc_OverflowError)) { | ||
PyErr_Format(PyExc_ValueError, | ||
"'id' must be a non-negative int, got %R", orig); | ||
} | ||
if (id == -1 && PyErr_Occurred()) { | ||
return -1; | ||
} | ||
if (id < 0) { | ||
|
@@ -202,23 +198,26 @@ interpid_richcompare(PyObject *self, PyObject *other, int op) | |
interpid *otherid = (interpid *)other; | ||
equal = (id->id == otherid->id); | ||
} | ||
else { | ||
other = PyNumber_Long(other); | ||
if (other == NULL) { | ||
PyErr_Clear(); | ||
Py_RETURN_NOTIMPLEMENTED; | ||
} | ||
int64_t otherid = PyLong_AsLongLong(other); | ||
Py_DECREF(other); | ||
if (otherid == -1 && PyErr_Occurred() != NULL) { | ||
else if (PyLong_CheckExact(other)) { | ||
/* Fast path */ | ||
int overflow; | ||
long long otherid = PyLong_AsLongLongAndOverflow(other, &overflow); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use int64_t or the macro? |
||
if (otherid == -1 && PyErr_Occurred()) { | ||
return NULL; | ||
} | ||
if (otherid < 0) { | ||
equal = 0; | ||
} | ||
else { | ||
equal = (id->id == otherid); | ||
equal = !overflow && (otherid >= 0) && (id->id == otherid); | ||
} | ||
else if (PyNumber_Check(other)) { | ||
PyObject *pyid = PyLong_FromLongLong(id->id); | ||
if (pyid == NULL) { | ||
return NULL; | ||
} | ||
PyObject *res = PyObject_RichCompare(pyid, other, op); | ||
Py_DECREF(pyid); | ||
return res; | ||
} | ||
else { | ||
Py_RETURN_NOTIMPLEMENTED; | ||
} | ||
|
||
if ((op == Py_EQ && equal) || (op == Py_NE && !equal)) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a questionable behavior to me.