gh-126061: Add PyLong_IsPositive/Zero/Negative() functions #126065

rruuaanng · 2024-10-28T07:48:32Z

Issue: Add PyLong_IsPositive/Zero/Negative() functions #126061

📚 Documentation preview 📚: https://cpython-previews--126065.org.readthedocs.build/

skirpichev

This should have tests and an entry in Doc/whatsnew/3.14.rst

Doc/c-api/long.rst

Include/longobject.h

ZeroIntensity · 2024-10-28T09:46:55Z

I'll get to reviewing this sometime today.

skirpichev · 2024-10-28T09:52:09Z

@rruuaanng, please avoid force-pushes.

rruuaanng · 2024-10-28T09:55:17Z

Sorry, there are a lot of changes that need to be rolled back. I will pay attention. :(

skirpichev · 2024-10-28T09:57:52Z

On Mon, Oct 28, 2024 at 02:55:40AM -0700, RUANG (James Roy) wrote: Sorry, there are a lot of changes that need to be rolled back. I will pay attention. ：（

In the end - individual commits will be squashed anyway.

ZeroIntensity

Nitpick: Use op for names, not obj

Objects/longobject.c

Include/cpython/longobject.h

Doc/whatsnew/3.14.rst

Doc/c-api/long.rst

rruuaanng · 2024-10-29T00:41:24Z

Given the number of comments,I'll reply here. I apologize for any inconvenience. The error handling implementation mentioned above was based on discussions in Capigroup, as victor expected. It'sItsavior should be consistent with GetSign. Regarding the comments, their style follows the format used in the same file. So, I apologize, but I may not make changes to them.

Edit
about the document, I'll change it.

ZeroIntensity · 2024-10-29T00:43:48Z

The error handling implementation mentioned above was based on discussions in Capigroup

Yes, I'm not asking to change how errors are handled. I'm worried about the error message.

skirpichev · 2024-10-29T02:22:47Z

Nitpick: Use op for names, not obj

Looking on other functions, rather v. But I don't think it's a big deal.

Co-authored-by: Peter Bierma <[email protected]>

serhiy-storchaka

LGTM in general. Please add tests. Look at Modules/_testcapi/long.c and Lib/test/test_capi/test_long.py.

Objects/longobject.c

Doc/c-api/long.rst

ZeroIntensity

I do see Serhiy's point of using the type name rather than the repr, but it's nice when "invalid type" errors are more unified, so I would prefer if it matched what was raised when using PyArg*. AFAICS, the only other function using the current message is PyLong_GetSign.

skirpichev · 2024-10-29T11:35:01Z

AFAICS, the only other function using the current message is PyLong_GetSign.

No. PyLong_AsNativeBytes, PyLong_AsUnsignedLongLong, PyLong_AsDouble, PyLong_AsSsize_t, etc.

ZeroIntensity · 2024-10-29T11:39:45Z

Ah, I see it in PyLong_AsNativeBytes, but that looks like the only other location. Could you point me to where it's thrown in the other functions? (GitHub search might be acting up.)

Modules/_testcapi/long.c

Lib/test/test_capi/test_long.py

skirpichev · 2024-10-29T11:47:51Z

On Tue, Oct 29, 2024 at 04:40:08AM -0700, Peter Bierma wrote: Ah, I see it in PyLong_AsNativeBytes, but that looks like the only other location. Could you point me to where it's thrown in the other functions? (GitHub search might be acting up.)

Correct phrase to grep is "an integer is required" ;)

ZeroIntensity · 2024-10-29T11:52:43Z

Ok, so that's not expect int, got type. Let's change the error to an integer is required (or better yet an integer is required, but got %T)

rruuaanng · 2024-10-29T11:55:12Z

I've made changes :)

erlend-aasland · 2024-10-29T12:05:07Z

Ok, so that's not expect int, got type. Let's change the error to an integer is required (or better yet an integer is required, but got %T)

See git grep "expected.*got". I think it would be fine to just change the word "expect" to "expected".

vstinner · 2024-11-07T13:29:09Z

@rruuaanng rruuaanng force-pushed the gh126061 branch from f5a2e49 to b1b82ff

@rruuaanng: What are you doing? You modified errnomodule.c!? I remove my approval.

rruuaanng · 2024-11-07T13:29:23Z

Sorry, there was something wrong with my terminal and it didn't update my current branch in real time after switching branches. I accidentally submitted another PR to my current branch. I rolled it back, oh :(

git push --force was used after my approval

ZeroIntensity

This is why we don't force push. It looks like the changes got rolled back, though.

rruuaanng · 2024-11-07T13:53:02Z

This isn't what I expected. Actually, I didn't notice that this event occurred at first, but later I didn't receive any emails from my target branch. After using checkout, my terminal still shows the name of the current PR branch instead of the target branch I wanted to switch to. This hasn't happened before, and I don't know why this event occurred (I will never use custom scripts from hackers again, it makes me feel very ashamed).

I apologize for the malfunction that occurred during the process.
:(

vstinner

LGTM

skirpichev · 2024-11-09T11:16:36Z

@encukou, are you ok with current wording?

picnixz

Should we update Doc/data/refcounts.dat even though the file is not maintained anymore?

Misc/NEWS.d/next/C_API/2024-10-28-15-56-03.gh-issue-126061.Py51_1.rst

picnixz · 2024-11-11T00:29:42Z

Doc/c-api/long.rst

@@ -571,6 +571,39 @@ distinguished from a number.  Use :c:func:`PyErr_Occurred` to disambiguate.
   .. versionadded:: 3.14


+.. c:function:: int PyLong_IsPositive(PyObject *obj)
+
+   Check if the integer object *obj* is positive (``obj > 0``).


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.

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.

I don't think such details are documented for other functions.

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.

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).

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.

Generally (unless documented otherwise), the C API has undefined behaviour if you pass in a NULL PyObject *. Usually it'll crash. Sometimes we assert that it's not NULL: that would probably be best here. But we don't need an if.

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.

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.

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".

Ok, no docs changes. I'll keep this comment unresolved, in case we want handle obj==NULL case in functions as suggested.

Doc/c-api/long.rst

skirpichev

Added few suggestions to address @picnixz review.

skirpichev · 2024-11-11T02:15:04Z

Objects/longobject.c

@@ -769,6 +769,36 @@ PyLong_AsUnsignedLongMask(PyObject *op)
    return val;
 }

+int
+PyLong_IsPositive(PyObject *obj)
+{


Suggested change

{

{

if (obj == NULL) {

PyErr_BadInternalCall();

return -1;

}

skirpichev · 2024-11-11T02:15:17Z

Objects/longobject.c

+
+int
+PyLong_IsNegative(PyObject *obj)
+{


Suggested change

{

{

if (obj == NULL) {

PyErr_BadInternalCall();

return -1;

}

skirpichev · 2024-11-11T02:15:32Z

Objects/longobject.c

+
+int
+PyLong_IsZero(PyObject *obj)
+{


Suggested change

{

{

if (obj == NULL) {

PyErr_BadInternalCall();

return -1;

}

skirpichev · 2024-11-11T02:17:06Z

Lib/test/test_capi/test_long.py

+
+        # CRASHES ispositive(NULL)


Suggested change

# CRASHES ispositive(NULL)

self.assertRaises(SystemError, ispositive, NULL)

skirpichev · 2024-11-11T02:17:32Z

Lib/test/test_capi/test_long.py

+
+        # CRASHES isnegative(NULL)


Suggested change

# CRASHES isnegative(NULL)

self.assertRaises(SystemError, isnegative, NULL)

skirpichev · 2024-11-11T02:17:55Z

Lib/test/test_capi/test_long.py

+
+        # CRASHES iszero(NULL)


Suggested change

# CRASHES iszero(NULL)

self.assertRaises(SystemError, iszero, NULL)

encukou

@encukou, are you ok with current wording?

Yes.

Should we update Doc/data/refcounts.dat even though the file is not maintained anymore?

Yeah, ideally yes.

…1_1.rst Co-authored-by: Bénédikt Tran <[email protected]>

vstinner · 2024-11-12T13:21:48Z

Merged, thanks @rruuaanng.

I suggest to write a separated PR to update Doc/data/refcounts.dat for these functions but also for other recently added functions. @rruuaanng: Do you want to write such PR?

@skirpichev: I disagree with checking for NULL. I prefer to have undefined behavior for best performance and document that the argument must not be NULL.

rruuaanng · 2024-11-12T13:26:08Z

Thanks for the merger of @vstinner :)

I suggest to write a separated PR to update Doc/data/refcounts.dat for these functions but also for other recently added functions. @rruuaanng: Do you want to write such PR?

Of course, for another PR, I will finish it.

skirpichev · 2024-11-12T13:39:59Z

I'll prepare a backport for pythoncapi-compat.

bedevere-app bot mentioned this pull request Oct 28, 2024

Add PyLong_IsPositive/Zero/Negative() functions #126061

Closed

skirpichev reviewed Oct 28, 2024

View reviewed changes

Doc/c-api/long.rst Outdated Show resolved Hide resolved

Doc/c-api/long.rst Outdated Show resolved Hide resolved

Include/longobject.h Outdated Show resolved Hide resolved

Add PyLong_IsPositive/Zero/Negative() functions

207a24f

rruuaanng force-pushed the gh126061 branch from ab0bd20 to 207a24f Compare October 28, 2024 09:07

ZeroIntensity self-requested a review October 28, 2024 09:46

skirpichev self-requested a review October 28, 2024 09:51

ZeroIntensity reviewed Oct 29, 2024

View reviewed changes

skirpichev requested a review from serhiy-storchaka October 29, 2024 02:24

skirpichev and others added 2 commits October 29, 2024 05:28

Apply suggestions from code review

6bbb376

Co-authored-by: Peter Bierma <[email protected]>

Add author

7436752

serhiy-storchaka reviewed Oct 29, 2024

View reviewed changes

Objects/longobject.c Outdated Show resolved Hide resolved

erlend-aasland reviewed Oct 29, 2024

View reviewed changes

Doc/c-api/long.rst Outdated Show resolved Hide resolved

rruuaanng added 2 commits October 29, 2024 19:13

Add test

1460879

Remove a space

b6a9126

ZeroIntensity reviewed Oct 29, 2024

View reviewed changes

skirpichev reviewed Oct 29, 2024

View reviewed changes

Change test

71bf57b

rruuaanng force-pushed the gh126061 branch from f5a2e49 to b1b82ff Compare November 7, 2024 13:27

bedevere-app bot added awaiting core review and removed awaiting merge labels Nov 7, 2024

skirpichev requested review from skirpichev and ZeroIntensity November 7, 2024 13:33

skirpichev approved these changes Nov 7, 2024

View reviewed changes

ZeroIntensity approved these changes Nov 7, 2024

View reviewed changes

rruuaanng requested a review from vstinner November 8, 2024 02:43

vstinner approved these changes Nov 8, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Nov 8, 2024

LeoniClemente mentioned this pull request Nov 11, 2024

<spam> #126656

Closed

picnixz reviewed Nov 11, 2024

View reviewed changes

skirpichev reviewed Nov 11, 2024

View reviewed changes

encukou approved these changes Nov 11, 2024

View reviewed changes

skirpichev and others added 2 commits November 11, 2024 17:09

Update Misc/NEWS.d/next/C_API/2024-10-28-15-56-03.gh-issue-126061.Py5…

5e7e6ce

…1_1.rst Co-authored-by: Bénédikt Tran <[email protected]>

Add assert

43cfbb4

erlend-aasland approved these changes Nov 11, 2024

View reviewed changes

vstinner merged commit 8ff7efb into python:main Nov 12, 2024
39 checks passed

bedevere-app bot removed the awaiting merge label Nov 12, 2024

rruuaanng deleted the gh126061 branch November 13, 2024 14:54

rruuaanng mentioned this pull request Nov 13, 2024

Add recently added functions to refcounts.dat #126788

Draft

-{
+{
+    if (obj == NULL) {
+        PyErr_BadInternalCall();
+        return -1;
+    }


	# CRASHES ispositive(NULL)
	self.assertRaises(SystemError, ispositive, NULL)


	# CRASHES isnegative(NULL)
	self.assertRaises(SystemError, isnegative, NULL)


	# CRASHES iszero(NULL)
	self.assertRaises(SystemError, iszero, NULL)

gh-126061: Add PyLong_IsPositive/Zero/Negative() functions #126065

gh-126061: Add PyLong_IsPositive/Zero/Negative() functions #126065

Conversation

rruuaanng commented Oct 28, 2024 • edited by github-actions bot Loading

skirpichev left a comment

Choose a reason for hiding this comment

ZeroIntensity commented Oct 28, 2024

skirpichev commented Oct 28, 2024

rruuaanng commented Oct 28, 2024 • edited Loading

skirpichev commented Oct 28, 2024 via email

ZeroIntensity left a comment

Choose a reason for hiding this comment

rruuaanng commented Oct 29, 2024 • edited Loading

ZeroIntensity commented Oct 29, 2024

skirpichev commented Oct 29, 2024

serhiy-storchaka left a comment

Choose a reason for hiding this comment

ZeroIntensity left a comment

Choose a reason for hiding this comment

skirpichev commented Oct 29, 2024

ZeroIntensity commented Oct 29, 2024

skirpichev commented Oct 29, 2024 via email • edited Loading

ZeroIntensity commented Oct 29, 2024

rruuaanng commented Oct 29, 2024

erlend-aasland commented Oct 29, 2024

vstinner commented Nov 7, 2024

rruuaanng commented Nov 7, 2024 • edited Loading

ZeroIntensity left a comment

Choose a reason for hiding this comment

rruuaanng commented Nov 7, 2024

vstinner left a comment

Choose a reason for hiding this comment

skirpichev commented Nov 9, 2024

picnixz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skirpichev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

encukou left a comment

Choose a reason for hiding this comment

vstinner commented Nov 12, 2024

rruuaanng commented Nov 12, 2024

skirpichev commented Nov 12, 2024

rruuaanng commented Oct 28, 2024 •

edited by github-actions bot

Loading

rruuaanng commented Oct 28, 2024 •

edited

Loading

rruuaanng commented Oct 29, 2024 •

edited

Loading

skirpichev commented Oct 29, 2024 via email •

edited

Loading

rruuaanng commented Nov 7, 2024 •

edited

Loading

picnixz left a comment •

edited

Loading

picnixz Nov 11, 2024 •

edited

Loading