Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-41790: update error in documentation #22242

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Sep 14, 2020

@koubaa
Copy link
Contributor Author

koubaa commented Sep 14, 2020

@vstinner @corona10 @shihai1991 I'm sure I have the wrong issue number but wanted to propose this change to the doc. I can change the number and rebase.

@vstinner
Copy link
Member

@vstinner @corona10 @shihai1991 I'm sure I have the wrong issue number but wanted to propose this change to the doc. I can change the number and rebase.

You have the wrong issue number.

@koubaa koubaa changed the title bpo-1635741: update error in documentation bpo-41790: update error in documentation Sep 15, 2020
@koubaa
Copy link
Contributor Author

koubaa commented Sep 15, 2020

@vstinner @corona10 @shihai1991 I'm sure I have the wrong issue number but wanted to propose this change to the doc. I can change the number and rebase.

You have the wrong issue number.

@vstinner I made a new issue in that case.

Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@koubaa
Copy link
Contributor Author

koubaa commented Sep 20, 2020

@vstinner is anything else needed before merging this?

@koubaa
Copy link
Contributor Author

koubaa commented Nov 15, 2020

@vstinner bump

declared. The sole exception are the type objects; since these must never be
deallocated, they are typically static :c:type:`PyTypeObject` objects.
declared. The sole exception are the type objects; they may be static
:c:type:`PyTypeObject` objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The sole exception are the type objects" looks wrong to me. Almost all types in Python are allocated on the heap.

Only static types are never deallocated.

I'm not sure why these kinds of technical details are giving in an introduction. It would be better to move them to https://docs.python.org/dev/c-api/refcounting.html no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least, copied there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I am not find the docs of PyHeapTypeObject. Maybe add those description info in it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain what to do with this change. The whole paragraph could be revisited, as Victor hints to. I'm not sure if this change makes things clearer for the reader.

@encukou: thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be technically correct, it's not just types – PyModuleDef is also a PyObject* (though I assume that's for historical reasons only). But that's too much detail here.

Comment on lines +239 to +240
declared. The sole exception are the type objects; they may be static
:c:type:`PyTypeObject` objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work?

Suggested change
declared. The sole exception are the type objects; they may be static
:c:type:`PyTypeObject` objects.
declared. (There are a few exceptions, like static type objects, but even those
are usually manipulated via pointers.)

Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has merge conflicts now.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants