Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
benjaminp merged 2 commits intopython:masterfromjdemeyer:pep590_tp_print
May 31, 2019

Conversation

@jdemeyer
Copy link
Contributor

@jdemeyerjdemeyer commentedMay 21, 2019
edited
Loading

Replacetp_print bytp_vectorcall_offset in comments when defining type structures, in all cases where the value is 0. While we're at it, also replacetp_reserved ->tp_as_async (this was never done systematically) andtp_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

@jdemeyerjdemeyer changed the titlebpo-36995: tp_print -> tp_vectorcall_offset and tp_reserved -> tp_as_asyncbpo-36974: tp_print -> tp_vectorcall_offset and tp_reserved -> tp_as_asyncMay 22, 2019
@jdemeyerjdemeyer marked this pull request as ready for reviewMay 22, 2019 12:58
@encukou
Copy link
Member

As you can see from all thosetp_reserved lying around, we don't do large clean-ups lightly. They interfere with backports and introduce noise ingit 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 (seeasyncio.Future for an aptly named example). Those avoid unusedand wrong field names.

@jdemeyer
Copy link
ContributorAuthor

As you can see from all thosetp_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 (seeasyncio.Future for an aptly named example). Those avoid unusedand 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 andgit blame noise than this PR.

@encukou
Copy link
Member

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

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

@jdemeyer
Copy link
ContributorAuthor

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 aPyTypeObject structure, as that typically changes behavior of a class. Also, the changes are sparse, so even if aPyTypeObject structure is changed slightly, git is likely to figure out the cherry-pick automatically.
  • git blame: the only lines which are changed involvetp_as_async andtp_vectorcall_offset, sogit 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
Copy link
ContributorAuthor

CC@benjaminp because you merged#9093.

@markshannon
Copy link
Member

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

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

@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 changePyTypeObject 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" onhttps://bugs.python.org to track the change. People will help, sure.

@benjaminp
Copy link
Contributor

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

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 mentionedtp_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
Copy link
ContributorAuthor

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

I disagree because of the reasons that@encukou mentioned.

@benjaminp
Copy link
Contributor

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.

Automatically replacetp_print -> tp_vectorcall_offsettp_compare -> tp_as_asynctp_reserved -> tp_as_async
@tiran
Copy link
Member

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

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

@jdemeyerjdemeyer mentioned this pull requestMay 30, 2019
@jdemeyer
Copy link
ContributorAuthor

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.

@benjaminpbenjaminp merged commit530f506 intopython:masterMay 31, 2019
DinoV pushed a commit to DinoV/cpython that referenced this pull requestJan 14, 2020
…async (pythonGH-13464)Automatically replacetp_print -> tp_vectorcall_offsettp_compare -> tp_as_asynctp_reserved -> tp_as_async
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@methanemethanemethane approved these changes

@1st11st1Awaiting requested review from 1st1

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag

@rhettingerrhettingerAwaiting requested review from rhettinger

@tirantiranAwaiting requested review from tiran

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jdemeyer@encukou@methane@markshannon@asvetlov@benjaminp@tiran@the-knights-who-say-ni@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp