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.

Sign up for GitHub

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

Jump to bottom

bpo-43244: Move pyarena.h from include/cpython to include/internal/. #24688

Closed
wants to merge 1 commit into from
Closed

bpo-43244: Move pyarena.h from include/cpython to include/internal/. #24688

wants to merge 1 commit into from

Conversation

shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Mar 1, 2021 •
edited by bedevere-bot
Loading

@shihai1991 shihai1991 changed the title bpo-43244: Move pyarena.h from include/cpython to include/internal/. [WIP] bpo-43244: Move pyarena.h from include/cpython to include/internal/. Mar 2, 2021
@shihai1991 shihai1991 force-pushed the bpo_43244 branch 2 times, most recently from be8eda6 to 5fefa0a Compare March 2, 2021 05:08
@markshannon
Copy link
Member

Isn't include/cpython already private, in terms of API?

@shihai1991
Copy link
Member Author

Isn't include/cpython already private, in terms of API?

IIRC, no limited apis store in include/cpython. Stable ABIs store in include. internal apis store in include/internal. cc @vstinner
The user can calling the no limited apis by including the python.h.

@shihai1991 shihai1991 changed the title [WIP] bpo-43244: Move pyarena.h from include/cpython to include/internal/. bpo-43244: Move pyarena.h from include/cpython to include/internal/. Mar 2, 2021
@shihai1991 shihai1991 requested a review from vstinner March 2, 2021 16:32
vstinner
vstinner requested changes Mar 8, 2021
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Include/Python-ast.h is blocking this PR.

Include/Python-ast.h
@@ -8,6 +8,7 @@ extern "C" {

#ifndef Py_LIMITED_API
#include "asdl.h"
#include "pycore_arena.h"
Copy link
Member

Choose a reason for hiding this comment

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

This is a blocker issue. You must not include an internal header file in a public header file. So I understand that if we want to implement https://bugs.python.org/issue43244 we should first move Python-ast.h to the internal C API.

Copy link
Member Author

Choose a reason for hiding this comment

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

You must not include an internal header file in a public header file.

Oh, got it. I haven't pay attention to this detail before.

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

@vstinner
Copy link
Member

I'm reworking the code base to be able to solve https://bugs.python.org/issue43244

But this PR is incorrect, so I close it. I may rewrite it. But other stuff should be done before (like moving Python-ast.h).

@vstinner vstinner closed this Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner requested changes

@isidentical isidentical Awaiting requested review from isidentical

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

@markshannon markshannon Awaiting requested review from markshannon

@pablogsal pablogsal Awaiting requested review from pablogsal

Assignees
No one assigned
Labels
awaiting changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shihai1991 @markshannon @bedevere-bot @vstinner @the-knights-who-say-ni