gh-129149: Create a macro PYLONG_FROM_SIGNED, and leverage it in functions

[srinivasreddy]

gh-129149: Create a macro PYLONG_FROM_SIGNED, and leverage it in functions PyLong_FromLong, PyLong_FromLongLong and PyLong_FromSsize_t so that repetitive code is removed

[skirpichev]

Test failures seems relevant. BTW, if you include PyLong_FromSsize_t change - please add news entry.

[picnixz]

• I don't think this should be under the same issue as it's a refactoring issue and not a performance issue.
• I don't think this adds value and there seems to be undefined behaviours here and there caught by GHA.

I would prefer not changing the current code. It works and there is no need to have a macro which would add useless tests/casts for some functions.

[skirpichev]

it's a refactoring issue and not a performance issue

Py_ssize_t case might be.

[picnixz]

Py_ssize_t case might be.

In this case, I think we should only focus on the Py_ssize_t case and don't need to convert it into a macro.

[chris-eibl]

Py_ssize_t case might be.

In this case, I think we should only focus on the Py_ssize_t case and don't need to convert it into a macro.

Then, we would just introduce roughly the same code block for PyLong_FromSsize_t() a third time.

Sure, that will do it's job to get us the fast path for medium-size integers, but the macro would be nice to remove the repetitive code and get symmetry with the already existing PYLONG_FROM_UINT?

Those two macros would also be a pretty good fit for PyLong_FromInt32(), PyLong_FromUInt32() and PyLong_FromUInt64().
They do the conversion totally different and have no fast path for medium-sized integers, too.

IMHO, all of those PyLong_From* functions are easier to reason about and maintain if they share the same code base.

Then, there are less spots to consider when altering them.

After all, I think this might be the reason why the fast path for medium-size integers was not done for all of them?

[picnixz]

Well.., I wouldn't mind if the 3 functions shared the exact same implementation, but it appears that there is an issue on Windows platforms. I actually don't mind the macro form but I'm -0 (I'm enclined to say yes because it would add symmetry but I'm also enclined to say no because overflow checks are apparently a bit different for ssize_t).

[chris-eibl]

Test failures seems relevant. BTW, if you include PyLong_FromSsize_t change - please add news entry.

issue on Windows

They most probably relate to

        size_t abs_ival;  /* Use size_t for unsigned operations */ \
        size_t t; \

which IMHO should read

        unsigned INT_TYPE abs_ival, t;  /* Use size_t for unsigned operations */ \

Which will make the macro harder to use for PyLong_FromSsize_t(), but this could be solved by using two parameters like UNSIGNED_INT_TYPE and INT_TYPE.

Another option might be, to implement PyLong_FromSsize_t() using either PyLong_FromLong() or PyLong_FromLongLong() depending on the size of Py_ssize_t.

[picnixz]

but this could be solved by using two parameters like UNSIGNED_INT_TYPE and INT_TYPE.

Which in this case won't be symmetric anymore with the other macro. It would also affect readability. Currently, the code is fine as it is, albeit we're missing a fast path. We can add that fast path but we don't need to make it everything as macros. I guess we can use a macro for the long values but for ssize_t, I would prefer keeping it as is.

[chris-eibl]

They are not totally symmetric, anyway, because e.g. the signed and unsigned path are different in the fast path check:

        if (-(INT_TYPE)PyLong_MASK <= (ival) && (ival) <= (INT_TYPE)PyLong_MASK) { \
            return _PyLong_FromMedium((sdigit)(ival)); \

versus

        if ((ival) <= PyLong_MASK) { \
            return _PyLong_FromMedium((sdigit)(ival)); \
        } \

But they are very similar.

I trust you core devs here what to prefer (macro vs repetition).

[skirpichev]

In this case, I think we should only focus on the Py_ssize_t case and don't need to convert it into a macro.

I think it's a good opportunity to refactor repetitive code.

[chris-eibl]

How shall we proceed?

@srinivasreddy do you plan to fix the errors?
Shall I post a code block here that compiles and passes the tests on my machine?

Or start over in a new PR?

[srinivasreddy]

@chris-eibl please feel free to start afresh .

[chris-eibl]

Done: #129301