Skip to content

Conversation

@markshannon
Copy link
Member

@markshannon markshannon commented Jun 13, 2019

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
Copy link
Contributor

jdemeyer commented Jun 14, 2019

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
Copy link
Contributor

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
Copy link
Member Author

markshannon commented Jun 17, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

encukou commented Jun 24, 2019

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
Copy link
Contributor

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
Copy link
Contributor

jdemeyer commented Jul 4, 2019

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
Copy link
Member

encukou commented Jul 5, 2019

Yes.

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

@encukou encukou closed this Jul 5, 2019
@markshannon markshannon deleted the fix-sign-nargs-pep590 branch March 23, 2022 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants