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

[rruuaanng]

• 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

[ZeroIntensity]

I'll get to reviewing this sometime today.

[skirpichev]

@rruuaanng, please avoid force-pushes.

[rruuaanng]

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

[skirpichev]

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

[rruuaanng]

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]

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]

Nitpick: Use op for names, not obj

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

[serhiy-storchaka]

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

[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]

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

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

[ZeroIntensity]

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

[skirpichev]

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]

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]

I've made changes :)

[erlend-aasland]

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

[rruuaanng]

The # CRACHES is...(NULL) comments seem to be wrong, as it does not appear such calls cause a crash. Can you confirm?

It exists only to keep in sync with test_long_getsign :)

[devdanzin]

It exists only to keep in sync with test_long_getsign :)

Ah, it's probably a way to signal that PyLong_IsPositive(NULL); crashes then? Anyway, if the comment should stay, I suggest correcting the typo "CRACHES" -> "CRASHES".

[ZeroIntensity]

Per the comments from Sergey and Erlend, it should be expected, not expect.

[rruuaanng]

Per the comments from Sergey and Erlend, it should be expected, not expect.

I see, but I wonder if there is any reason to change GetSign as well.

[ZeroIntensity]

There could be, but that's for another PR. I'm also happy with the phrase an integer is required here for consistency, but I like displaying some information about the object because it's useful for debugging.

[rruuaanng]

an integer is required

Maybe it's too long? Maybe keeping expected int is enough.
(I think)

[rruuaanng]

There could be, but that's for another PR. I'm also happy with the phrase here for consistency, but I like displaying some information about the object because it's useful for debugging.an integer is required

That's say, we should change it to an integer is required? What should I do, can you give a diff.

Edit
It seems that the current one isn't bad.

expected int, got %T

stale

[ZeroIntensity]

I'm happy to approve this if you change it to an assertRaisesRegex like I asked.

[skirpichev]

it's very positive

Also it's very negative:D

Lets just stick with math terminolgy.

[rruuaanng]

I'm happy to approve this if you change it to an assertRaisesRegex like I asked.

Sorry peter. I'm not sure we need to be consistent with the test for GetSign, changing the current test to assertRaisesRegex format seems inconsistent with the style. Maybe ask Sergey for his opinion. I'm happy to change the code, but I'm not sure if others would object to it.

[ZeroIntensity]

I'm not sure we need to be consistent with the test for GetSign, changing the current test to assertRaisesRegex format seems inconsistent with the style.

No, assertRaisesRegex has nothing to do with the style of GetSign. It's to make sure that the TypeError stays correct and displays the proper message (because in theory, something else could raise an exception, but since we've documented that as the only way it can fail, that would be wrong).

[rruuaanng]

Emmm..., Look at this, actually my point is the same. I think we just need to identify a problem, and it will trigger this when the type is wrong.
https://github.com/python/cpython/pull/126065/files#r1825465408

[vstinner]

If you dislike strictly, I suggest:

Suggested change

	Check if the integer object *obj* is positive.
	Check if the integer object *obj* is positive: greater than zero.

[skirpichev]

I don't feel this addition is needed.

But at least this form is less confusing than "strictly positive". Maybe: "Check if the integer object obj is positive, i.e. greater than zero."

[skirpichev]

@serhiy-storchaka, I think this is ready for final review.

[ZeroIntensity]

OK, I've ran out of things to nitpick 😄

I'm not particularly happy about the tests, but in hindsight it's probably fine. Though, if we break it in the future don't say I didn't warn you!

Thank you @rruuaanng for the responsiveness, and thank you @skirpichev for all the technical insight (and for dealing with my pickiness)!