Add public functions PyLong_IsPositive(), PyLong_IsNegative() and PyLong_IsZero()

[serhiy-storchaka]

int PyLong_IsPositive(PyObject *);
int PyLong_IsNegative(PyObject *);
int PyLong_IsZero(PyObject *);

They should be identical to existing private functions _PyLong_IsPositive(), _PyLong_IsNegative() and _PyLong_IsZero(), except that the argument is PyObject * instead of PyLongObject *.

The private functions are much more used in the CPython code than _PyLong_Sign (numbers are not accurate):

26 _PyLong_IsZero
 7 _PyLong_IsPositive
71 _PyLong_IsNegative
 8 _PyLong_Sign

Some of cases that use _PyLong_Sign could use _PyLong_IsNegative etc instead. It is expected that they will be more used in the user code too.

They are faster than PyLong_GetSign because they have less code.

They are also more convenient, because you do not need to introduce a variable for result and can use them in expression. For example:

if (PyLong_IsNegative(obj)) {

vs

int sign;
(void)PyLong_GetSign(obj, &sign);
if (sign < 0) {

[vstinner]

What are return values? Especially when the argument is not a Python int?

[serhiy-storchaka]

1 for true, 0 for false.

They should be only used with a Python int, like PyUnicode_IsIdentifier() can be only used with a Python str.

[skirpichev]

0 for false

You meant for non-int's too, right?

They should be only used with a Python int

Then using PyLongObject seems more logical. If so, I don't see why this is better than single

int PyLong_Sign(PyLongobject *obj);

interface, already rejected in #19

PS:

int sign;
(void)PyLong_GetSign(obj, &sign);
if (sign < 0) {

Some compilers (e.g. recent gcc) also emit warning on the first line.

[encukou]

New public API should take PyObject*, and raise TypeError on bad types. These don't do it. Is there any new reason to make an exception to that guideline? I'd hate to repeat the conversation in #19.

Most of the usage in CPython is in unicodeobject.c implemeting PyLong itself. I don't think that's representative for third-party usage.

The functions quite easy to implement using PyLong_GetSign. Is it really worth adding three extra API functions?

[serhiy-storchaka]

Even excluding Objects/longobject.c, there are much more use cases for these functions than for _PyLong_Sign (note that some of these cases for _PyLong_Sign can use _PyLong_IsNegative).

14 _PyLong_IsZero
 2 _PyLong_IsPositive
24 _PyLong_IsNegative
 6 _PyLong_Sign

They are faster and more convenient. They are used in many different files:

Modules/posixmodule.c
Modules/mathmodule.c
Modules/_tkinter.c
Modules/_decimal/_decimal.c
Objects/typeobject.c
Objects/sliceobject.c
Objects/abstract.c
Objects/rangeobject.c
Python/structmember.c
Python/bytecodes.c
Python/optimizer_symbols.c
Python/ast_opt.c
Python/marshal.c

If you insist, we can make them returning 0 or setting an error and returning -1 if the argument is not a Python int. They will still be fast and convenient.

[vstinner]

New public API should take PyObject*, and raise TypeError on bad types. These don't do it. Is there any new reason to make an exception to that guideline? I'd hate to repeat the conversation in https://github.com/capi-workgroup/decisions/issues/19.

PyLong already has exceptions: PyUnstable_Long_IsCompact() and PyUnstable_Long_CompactValue() parameter type is const PyLongObject* instead of PyObject*.

Maybe PyLong_IsPositive(), PyLong_IsNegative() and PyLong_IsZero() belong to the PyUnstable API instead.

[serhiy-storchaka]

All uses outside of Objects/longobject.c currently require the argument to be cast to PyLongObject *. I am sure this will be the same in the user code.

When we use other pointer type than PyObject * in C, this makes the code less type safe.

[skirpichev]

Is it really worth adding three extra API functions?

As author of the PyLong_Sign() proposal, I would like to see these functions instead of the implemented PyLong_GetSign().

Original idea was to offer GMP-like interface - as mpz_sgn() - that take single PyLongObject* argument (or PyObject* and not raise errors on int's). That's just as convenient and readable as PyLong_Is* functions (at cost of one function, instead of several), e.g.:

if (PyLong_Sign(obj) < 0) {

But with PyLong_GetSign() - this is impossible.

With API like

/* Returns 1 if the object obj is negative integer, and 0 otherwise.   On failure,
   return -1 with an exception set.  This function always succeeds if obj is
   a PyLongObject or its subtype. */
int PyLong_IsNegative(PyObject obj);

most consumers could use one-liners as in the issue description and not worry about calling PyErr_Occurred().

The functions quite easy to implement using PyLong_GetSign.

That's true. But I worry that all consumers of that API will end in implementing such wrappers (or PyLong_Sign-like one).

The gmpy2 project has just one place, that will call this API and this doesn't matter. But not all C API users so lucky.

Even excluding Objects/longobject.c, there are much more use cases for these functions than for _PyLong_Sign (note that some of these cases for _PyLong_Sign can use _PyLong_IsNegative).

To me it looks, that across the whole CPython codebase - only three calls to the _PyLong_Sign() do make sense: floatobject.c, _pickle.c and sliceobject.c. All could be converted to using _PyLong_Is* without loss in readability and, I think, with minimal impact on performance.

[encukou]

That's true. But I worry that all consumers of that API will end in implementing such wrappers (or PyLong_Sign-like one). The gmpy2 project has just one place, that will call this API and this doesn't matter. But not all C API users so lucky.

For reference, which/how many other consumers are we talking about? I thought gmpy2 is the main user.

[serhiy-storchaka]

CPython itself uses such functions much more than the sign function.

[vstinner]

If the main consumer is CPython, we can use the internal C API, no?

[serhiy-storchaka]

CPython is just an example. Looking at the list of files where they are used you can see that they are usable in many different fields.

[vstinner]

As I wrote previously, if you want PyLongObject* argument, I suggest to add functions to the PyUnstable API.

[skirpichev]

I thought gmpy2 is the main user.

Probably, sage is. gmpy2 is much less popular project.

https://github.com/sagemath/sage/blob/b7dd28d2456307802ce4ddcfc692331744f9b0db/src/sage/libs/gmp/pylong.pyx#L95

CPython itself uses such functions much more than the sign function.

But for CPython sometimes _PyLong_Sign() does make sense, e.g.: https://github.com/python/cpython/blob/92893fd8dc803ed7cdde55d29d25f84ccb5e3ef0/Modules/_pickle.c#L2127

For external projects, I think that Positive/Zero/Negative predicates will handle all use cases.

[encukou]

Based on that link, it looks like sage uses this in combination with other private API, to import/export int data without conversion. We should add API for that whole use case, not for one part of it.

[skirpichev]

it looks like sage uses this in combination with other private API, to import/export int data without conversion.

Just as gmpy2 or everyone else.

We should add API for that whole use case, not for one part of it.

It's not easy to do this in one shot;) #31 address other parts, perhaps all.

[vstinner]

Based on that link, it looks like sage uses this in combination with other private API, to import/export int data without conversion. We should add API for that whole use case, not for one part of it.

I proposed a whole API to import-export Python int in https://github.com/capi-workgroup/decisions/issues/35.

[vstinner]

@serhiy-storchaka: You didn't reply to my previous question. Would you be ok with an unstable API with PyLongObject* parameter? It would avoid the need for error checking since we know that the argument is a Python int (or a subclass).

int PyUnstable_Long_IsPositive(PyLongObject *);
int PyUnstable_Long_IsNegative(PyLongObject *);
int PyUnstable_Long_IsZero(PyLongObject *);

[vstinner]

I would prefer a single int PyUnstable_Long_GetSign(PyLongObject *), you can use it directly in comparison:

int is_positive = (PyUnstable_Long_GetSign(obj) >= 0);
int is_zero = (PyUnstable_Long_GetSign(obj) == 0);
int is_negative = (PyUnstable_Long_GetSign(obj) < 0);

[serhiy-storchaka]

No, I do not want PyLongObject * argument. It is inconvenient to use outside of longobject.c and maybe even in it. Requiring a cast to PyLongObject * at every use of this API will just make the code less safe. It should be PyObject *, as in all public C API.

PyUnstable_Long_GetSign() would add more overhead.

As for the PyUnstable_ prefix, well, if it helps to bring this API to users.

[vstinner]

It should be PyObject *, as in all public C API.

In this case, I would require the caller to handle errors if the argument is not a Python int. Which is not the API that you are asking for.

No, I do not want PyLongObject * argument. It is inconvenient to use outside of longobject.c and maybe even in it.

The idea is that the caller is responsible to do the cast and so takes the responsibility of invalid casts.

[skirpichev]

In this case, I would require the caller to handle errors if the argument is not a Python int.

I think it's fine if we document that for ints (+subtypes) - no exceptions will be raised.

[vstinner]

So what we can have are these 3 functions:

int PyLong_IsPositive(PyObject *);
int PyLong_IsNegative(PyObject *);
int PyLong_IsZero(PyObject *);

• Returns 1 if the object obj is, respectively, positive, negative or zero.
• Returns 0 otherwise.
• If the argument is not a Python int, raise a TypeError and return -1.

So these functions always succeed if the argument is a Python int or its subtype.

@serhiy-storchaka @skirpichev: Would you be ok with such API?

[skirpichev]

Yep, this definitely better than PyLong_GetSign() API (which should be removed). Based on above statistics, maybe PyLong_IsNegative and PyLong_IsZero - enough.

[vstinner]

Ok, let me open a vote on these 3 functions (API):

• [x] @encukou
• [x] @erlend-aasland
• [x] @mdboom
• [x] @serhiy-storchaka
• [x] @vstinner
• [x] @zooba

Moreover, if we add these 3 functions, should we remove PyLong_GetSign()?

• Remove PyLong_GetSign():
  • [x] @encukou
  • [x] @erlend-aasland
  • [ ] @mdboom
  • [ ] @serhiy-storchaka
  • [ ] @vstinner
  • [x] @zooba
• Keep PyLong_GetSign():
  • [ ] @encukou
  • [ ] @erlend-aasland
  • [ ] @mdboom
  • [x] @serhiy-storchaka
  • [ ] @vstinner
  • [ ] @zooba

[encukou]

I still think that projects that need these more than once or twice should implement them using PyLong_GetSign, handling the four possible results in the way that's most appropriate for them.

[vstinner]

@zooba: Would you mind to vote on these APIs?

[zooba]

Done. I don't mind a soft/docs-only deprecation of GetSign, but I see no reason to remove it.

[vstinner]

@encukou: You're against adding these 3 functions, but other voters are in favor of the addition. Can you consider changing your vote? Same remark question for the keep or remove PyLong_GetSign() vote.

[skirpichev]

BTW, the sentence "I still think that projects that need these more than once or twice should implement them using PyLong_GetSign, handling the four possible results in the way that's most appropriate for them." contradicts to vote for removing the PyLong_GetSign().

[encukou]

Looks like I'm outvoted. I'll change my vote on the 3 functions, so I don't block progress. But:

I don't mind a soft/docs-only deprecation of GetSign, but I see no reason to remove it.

Did y'all consider that PyLong_GetSign is new in 3.14? We'd be (soft-)deprecating it in the same release where it's added.

[zooba]

Did y'all consider that PyLong_GetSign is new in 3.14?

No I didn't. If it's not been released, then yeah, just remove it.

[erlend-aasland]

A PR for adding PyLong_IsPositive, PyLong_IsNegative, and PyLong_IsZero is up for review:

• https://github.com/python/cpython/pull/126065

[skirpichev]

If it's not been released, then yeah, just remove it.

Then we have 3/3 votes for/against removing PyLong_GetSign(). What to do?

[encukou]

I have re-set the vote, and marked Steve, Erlend & me as wanting to remove it.

Does anyone want to keep PyLong_GetSign, even though it was only released in an alpha version?