PEP 590: Correct sign of return value of PyVectorcall_NARGS().

[markshannon]

The return type of PyVectorcall_NARGS() is documented at Py_ssize_t but it makes no sense for this to be a signed value, for two reasons.

1. The number of arguments must be a non-negative number; you cannot have a negative number of arguments.
2. The input value is an unsigned number. The conversion to a signed value may add some overhead. While the version returning a signed integer will probably be as efficient as the version returning a unsigned integer, there is no guarantee that it will be.

[jdemeyer]

I argue that it should remain Py_ssize_t for the simple reason that every length or size argument in CPython uses Py_ssize_t (I have no idea why but this PR is not the place to discuss that). It would be inconsistent to use size_t here but Py_ssize_t for METH_FASTCALL for example.

[jdemeyer]

For future reference: I think it's good to put all authors of a PEP in CC when proposing a change to a PEP.

[markshannon]

We all agreed that the size of nargs would be size_t, not Py_ssize_t, but when the change was made here: it omitted to change the return type, and thus introduced a unsigned to signed cast that wasn't previously present.
We should remove that cast for the reasons stated above.

[jdemeyer]

We all agreed that the size of nargs would be size_t, not Py_ssize_t, but when the change was made here: it omitted to change the return type, and thus introduced a unsigned to signed cast that wasn't previously present.

As you can see from #1066 (comment), this was intentional. We didn't forget to change the return type.

[jdemeyer]

2. The conversion to a signed value may add some overhead.

I don't see why. It's a cast from an unsigned number to a signed number of the same size. This doesn't change the bit pattern of the number, so why there should there be any overhead?

To support METH_FASTCALL, the cast is going to be needed at some point anyway since METH_FASTCALL takes its number of arguments as Py_ssize_t.

[markshannon]

Compare the GCC output for the form with and without the cast: https://www.godbolt.org/z/GG3Pmy
Of course, the exact compiler output will depend on the context, but this is an example of how the additional cast can produce worse code.

METH_FASTCALL is not part of the public API so we can fix it as well.
Passing signed types when the underlying value is unsigned is likely to introduce errors and inefficiencies and has no benefit.

[jdemeyer]

Compare the GCC output for the form with and without the cast: https://www.godbolt.org/z/GG3Pmy
Of course, the exact compiler output will depend on the context, but this is an example of how the additional cast can produce worse code.

Comparing unoptimized code is not very relevant. When you compile even with -O1, the generated code becomes identical (as it should be).

[jdemeyer]

Passing signed types when the underlying value is unsigned is likely to introduce errors and inefficiencies

Maybe you're right. I'm not even arguing about that. I'm just saying that using size_t here is inconsistent with everything else in CPython. If you want to use size_t in this very specific case when all other size/length parameters in CPython use Py_ssize_t, you need a good reason for that.

[jdemeyer]

Compare the GCC output for the form with and without the cast: https://www.godbolt.org/z/GG3Pmy

By the way: that looks like a very cool site, I didn't know about it. Thanks!

[encukou]

Indeed, Py_ssize_t is there for consistency, it's what CPython uses for all the other "argument counts". If we change it, let's change it everywhere.

FWIW, I grew to like the practical hack: as it's done now, you're likely to get a compiler warning when mixing "arguments with the offset flag" and "just number of arguments" inappropriately.

[jdemeyer]

FWIW, I grew to like the practical hack: as it's done now, you're likely to get a compiler warning when mixing "arguments with the offset flag" and "just number of arguments" inappropriately.

+1

[jdemeyer]

Can we get some resolution on this please? There is no point in keeping this PR open if it doesn't look like it will be accepted.

[encukou]

Yes.

Please re-open if there's more to discuss. (Or comment, if you can't reopen.)