-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-126061: Add PyLong_IsPositive/Zero/Negative() functions #126065
Changes from 18 commits
207a24f
6bbb376
7436752
1460879
b6a9126
71bf57b
04b23c7
a30a7f1
b2f3762
a18781f
c50d7a2
63adaf6
5ec2379
196cf36
73e150d
630bc01
daecbc9
b1b82ff
5e7e6ce
43cfbb4
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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -643,6 +643,51 @@ def test_long_getsign(self): | |||||||
|
||||||||
# CRASHES getsign(NULL) | ||||||||
|
||||||||
def test_long_ispositive(self): | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
# Test PyLong_IsPositive() | ||||||||
ispositive = _testcapi.pylong_ispositive | ||||||||
self.assertEqual(ispositive(1), 1) | ||||||||
self.assertEqual(ispositive(123), 1) | ||||||||
self.assertEqual(ispositive(-1), 0) | ||||||||
self.assertEqual(ispositive(0), 0) | ||||||||
self.assertEqual(ispositive(True), 1) | ||||||||
self.assertEqual(ispositive(False), 0) | ||||||||
self.assertEqual(ispositive(IntSubclass(-1)), 0) | ||||||||
self.assertRaises(TypeError, ispositive, 1.0) | ||||||||
self.assertRaises(TypeError, ispositive, Index(123)) | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
ZeroIntensity marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
# CRASHES ispositive(NULL) | ||||||||
Comment on lines
+658
to
+659
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.
Suggested change
|
||||||||
|
||||||||
def test_long_isnegative(self): | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
# Test PyLong_IsNegative() | ||||||||
isnegative = _testcapi.pylong_isnegative | ||||||||
self.assertEqual(isnegative(1), 0) | ||||||||
self.assertEqual(isnegative(123), 0) | ||||||||
self.assertEqual(isnegative(-1), 1) | ||||||||
self.assertEqual(isnegative(0), 0) | ||||||||
self.assertEqual(isnegative(True), 0) | ||||||||
self.assertEqual(isnegative(False), 0) | ||||||||
self.assertEqual(isnegative(IntSubclass(-1)), 1) | ||||||||
self.assertRaises(TypeError, isnegative, 1.0) | ||||||||
self.assertRaises(TypeError, isnegative, Index(123)) | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
# CRASHES isnegative(NULL) | ||||||||
Comment on lines
+673
to
+674
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.
Suggested change
|
||||||||
|
||||||||
def test_long_iszero(self): | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
# Test PyLong_IsZero() | ||||||||
iszero = _testcapi.pylong_iszero | ||||||||
self.assertEqual(iszero(1), 0) | ||||||||
self.assertEqual(iszero(-1), 0) | ||||||||
self.assertEqual(iszero(0), 1) | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
self.assertEqual(iszero(True), 0) | ||||||||
self.assertEqual(iszero(False), 1) | ||||||||
self.assertEqual(iszero(IntSubclass(-1)), 0) | ||||||||
self.assertEqual(iszero(IntSubclass(0)), 1) | ||||||||
self.assertRaises(TypeError, iszero, 1.0) | ||||||||
self.assertRaises(TypeError, iszero, Index(123)) | ||||||||
rruuaanng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
# CRASHES iszero(NULL) | ||||||||
Comment on lines
+688
to
+689
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.
Suggested change
|
||||||||
|
||||||||
def test_long_asint32(self): | ||||||||
# Test PyLong_AsInt32() and PyLong_FromInt32() | ||||||||
to_int32 = _testlimitedcapi.pylong_asint32 | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Add :c:func:`PyLong_IsPositive`, :c:func:`PyLong_IsNegative` | ||
and :c:func:`PyLong_IsZero` for checking if :c:type:`PyLongObject` | ||
is positive, negative, or zero, respectively. | ||
skirpichev marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -769,6 +769,36 @@ PyLong_AsUnsignedLongMask(PyObject *op) | |||||||||||||
return val; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
int | ||||||||||||||
PyLong_IsPositive(PyObject *obj) | ||||||||||||||
{ | ||||||||||||||
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.
Suggested change
|
||||||||||||||
if (!PyLong_Check(obj)) { | ||||||||||||||
PyErr_Format(PyExc_TypeError, "expected int, got %T", obj); | ||||||||||||||
return -1; | ||||||||||||||
} | ||||||||||||||
return _PyLong_IsPositive((PyLongObject *)obj); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
int | ||||||||||||||
PyLong_IsNegative(PyObject *obj) | ||||||||||||||
{ | ||||||||||||||
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.
Suggested change
|
||||||||||||||
if (!PyLong_Check(obj)) { | ||||||||||||||
PyErr_Format(PyExc_TypeError, "expected int, got %T", obj); | ||||||||||||||
return -1; | ||||||||||||||
} | ||||||||||||||
return _PyLong_IsNegative((PyLongObject *)obj); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
int | ||||||||||||||
PyLong_IsZero(PyObject *obj) | ||||||||||||||
{ | ||||||||||||||
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.
Suggested change
|
||||||||||||||
if (!PyLong_Check(obj)) { | ||||||||||||||
PyErr_Format(PyExc_TypeError, "expected int, got %T", obj); | ||||||||||||||
return -1; | ||||||||||||||
} | ||||||||||||||
return _PyLong_IsZero((PyLongObject *)obj); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
int | ||||||||||||||
_PyLong_Sign(PyObject *vv) | ||||||||||||||
{ | ||||||||||||||
|
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.
AFAICT,
obj
must not be NULL since otherwise it crashes (it's not the same as setting an exception). Other functions in this section mention when an input must not be NULL.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.
I don't think such details are documented for other functions.
But maybe we should add instead the
NULL
check (like e.g. PyLong_AsNativeBytes). IIUC, there is no rule to do so for new functions.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.
There is: https://docs.python.org/3.14/c-api/long.html#c.PyLong_AsInt32. Those were recently added functions so that's why I suggested continuing specifying whether a NULL is allowed or not.
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.
In fact, PyLong_AsInt32 doesn't crashes on
NULL
. So,value must not be NULL
sentence is slightly redundant: in this case function set an exception and return-1
(exactly as specified right above).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.
Ah, maybe I wasn't clear but value in this case is the output buffer which is then dereferenced without a NULL check. It's not the PyObject input. My point was that when we do not expect a NULL value (and don't check for it), we specify it.
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.
Generally (unless documented otherwise), the C API has undefined behaviour if you pass in a NULL
PyObject *
. Usually it'll crash. Sometimes weassert
that it's not NULL: that would probably be best here. But we don't need anif
.I advise against adding a note in the docs about NULL, since it could hint (to humans) that one could pass NULL to other functions.
PyLong_AsInt32
is different: there the pointer is an “output argument” that the function fills in. An explicit note makes more sense there. Generally, for many functions like that, it's sometimes useful to run other effects of a function without getting the result filled in, and so some functions like this do accept NULL.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.
I see your point here. The alternative is to specify it to other functions explicitly as well. But it's probably better to just do it in one docs PR later if needed. So I don't mind not specifying "do not pass NULL".
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.
Ok, no docs changes. I'll keep this comment unresolved, in case we want handle obj==NULL case in functions as suggested.