bpo-36974: tp_print -> tp_vectorcall_offset and tp_reserved -> tp_as_async

[jdemeyer]

Replace tp_print by tp_vectorcall_offset in comments when defining type structures, in all cases where the value is 0. While we're at it, also replace tp_reserved -> tp_as_async (this was never done systematically) and tp_compare -> tp_as_async in a few left-over places.

This PR is independent from the rest of the PEP 590 implementation.

CC @encukou @markshannon

https://bugs.python.org/issue36974

[encukou]

As you can see from all those tp_reserved lying around, we don't do large clean-ups lightly. They interfere with backports and introduce noise in git log / git blame. And they touch a lot of code that has owners set, which is why this PR has so many reviewers set automatically.

If there's a type you're already changing, prefer C99 designated initializers (see asyncio.Future for an aptly named example). Those avoid unused and wrong field names.

[jdemeyer]

As you can see from all those tp_reserved lying around

It's not clear to me whether that's intentional or just an oversight. At least, tp_compare -> tp_reserved was mass changed. If you want to reject this PR, that's totally fine for me. I really don't care.

If there's a type you're already changing, prefer C99 designated initializers (see asyncio.Future for an aptly named example). Those avoid unused and wrong field names.

But then I guess I have to rewrite the type structure completely (I don't think it's sane to combine old-style and C99-style initializers in the same type structure), which is actually worse in terms of backportability and git blame noise than this PR.

[encukou]

It's not clear to me whether that's intentional or just an oversight. At least, tp_compare -> tp_reserved was mass changed.

It was, in 2009, for Python 3.0. That was a different time.

If you want to reject this PR, that's totally fine for me. I really don't care.

I'm not comfortable merging it.

C99 designated initializers

But then I guess I have to rewrite the type structure completely (I don't think it's sane to combine old-style and C99-style initializers in the same type structure), which is actually worse in terms of backportability and git blame noise than this PR.

Yes. That's why it's better done one type at a time.

[methane]

I don't have strong opinion here.

If we keep lying comment in many type definitions,
tp_vector_call_offset /* this was tp_print */ comment will be added in PyTypeObject.

[jdemeyer]

They interfere with backports and introduce noise in git log / git blame. And they touch a lot of code that has owners set, which is why this PR has so many reviewers set automatically.

Let's look at these 4 points individually:

• backports: it's unlikely that a bugfix PR involves changing a PyTypeObject structure, as that typically changes behavior of a class. Also, the changes are sparse, so even if a PyTypeObject structure is changed slightly, git is likely to figure out the cherry-pick automatically.
• git blame: the only lines which are changed involve tp_as_async and tp_vectorcall_offset, so git blame will only be affected on those lines. I don't see a problem here.
• git log: this does introduce changes in many files, true. But that's not so important, it's just a minor inconvenience.
• owners: so what? The changes don't affect the functioning of the modules, only comments. So the owners don't need to care.

[jdemeyer]

CC @benjaminp because you merged #9093.

[markshannon]

The problem with this PR is not the changes it makes, but that no one will fee comfortable merging it becauseit covers too much of the code base at once.
If you make PRs that one person can take responsibility for then they are much more likely to get merged.
So, can you start with a PR that covers just object, the callables and dicts. I'd be happy to merge that.

[jdemeyer]

it covers too much of the code base at once.

I don't understand why that's a problem for such a trivial PR like this one. I agree, as a general rule, one shouldn't put too many unrelated changes in one PR. But in the case, the changes are so trivial that I don't see the problem in having them all together.

one person can take responsibility for

I don't see why somebody should "take responsibility for" this PR. Maybe I just misunderstand what you mean here. This PR literally only changes comments, so it's extremely improbable that it will break anything.

[asvetlov]

@jdemeyer please keep calm.
Relax, breath in and out.
I understand and share @encukou and @markshannon objections.
109 changed files are too much, even if the change is about comments only.

Please split the PR into 109 ones for one module at once.
Even better, let's change PyTypeObject definitions to use a new syntax like

static PyTypeObject FutureType = {
    PyVarObject_HEAD_INIT(NULL, 0)
    "_asyncio.Future",
    sizeof(FutureObj),                       /* tp_basicsize */
    .tp_dealloc = FutureObj_dealloc,
    .tp_as_async = &FutureType_as_async,
    ...

Sorry, it takes much more work. Maybe some related modules can be addressed by single PR.
As a reviewer, I can overview the module change and approve it instantly if the scope of changes is narrow.

We can open "easy issue" on https://bugs.python.org to track the change. People will help, sure.

[benjaminp]

I also don't see what's objectionable about a large, automated comment-only change. We should review whatever the process was for making it and call it a day. Making 109 one-file PRs doesn't seems like a good use of anyone's time.

[jdemeyer]

It's not completely automated. Of course, I started with automatic replacements but then I made some minor adjustments (mostly whitespace and fixing a few comments which mentioned tp_reserved but which were no longer accurate). If you prefer, I can separate out the automatic replacements from the manual changes in two different commits.

[jdemeyer]

Even better, let's change PyTypeObject definitions to use a new syntax

I disagree because of the reasons that @encukou mentioned.

[benjaminp]

Yes, for large changes it's best practice to document how they were made in the commit message.

I still think this is fine. The current comments are wrong for those slots. So, even if there are a few things in here that aren't completely correct, it's a strict improvement.

[tiran]

I'm -0 on the change set. It's a lot of noise, but I won't object if the majority wants to move on with it.

[jdemeyer]

Yes, for large changes it's best practice to document how they were made in the commit message.

OK, I changed the commits. The first commit replaces everywhere:

• tp_print -> tp_vectorcall_offset
• tp_reserved -> tp_as_async
• tp_compare -> tp_as_async

The second commit adds some manual fixes, partially reverting the first one where not applicable (for example in docs, which I plan to fix in #13450).

[jdemeyer]

I suggest that this is merged either in 3.8 (i.e. very soon now) or not at all. If we merge this in 3.9 but not in 3.8, it's going to be worse for backportability, which is arguably more important in the beta phase than the release phase.