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

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

it's a refactoring issue and not a performance issue

Py_ssize_t case might be.

Py_ssize_t case might be.

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

Py_ssize_t case might be.

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

Then, we would just introduce roughly the same code block forPyLong_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 existingPYLONG_FROM_UINT?

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

IMHO, all of thosePyLong_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?

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

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_tabs_ival;/* Use size_t for unsigned operations */ \size_tt; \

which IMHO should read

unsignedINT_TYPEabs_ival,t;/* Use size_t for unsigned operations */ \

Which will make the macro harder to use forPyLong_FromSsize_t(), but this could be solved by using two parameters likeUNSIGNED_INT_TYPE andINT_TYPE.

Another option might be, to implementPyLong_FromSsize_t() using eitherPyLong_FromLong() orPyLong_FromLongLong() depending on the size ofPy_ssize_t.

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. Wecan 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 forssize_t, I would prefer keeping it as is.

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

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.

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?

@chris-eibl please feel free to start afresh .

Done:#129301