GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-35134: Move non-limited C API of include/compile.h into include/cpython. by shihai1991 · Pull Request #24922 · python/cpython

Conversation

@shihai1991

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue35134

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shihai1991
shihai1991 requested a review from a team as a code owner March 18, 2021 17:43
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 18, 2021
@bedevere-bot bedevere-bot added the awaiting review label Mar 18, 2021
@shihai1991

shihai1991 commented Mar 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@vstinner Hi, Victor. When you plan to rewrite pyarena.h, I think this compile.h should be updated too :)

But I am not sure the declaration of PyAST_XXX in compile.h can be moved to include/internal or not.
I checked several search results of PyAST_CompileEx in github.com and didn't find anybody calling this function directly.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner requested changes Mar 18, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It doesn't work, compile.h ends with a few API which are part of the limited C API:

/* These definitions must match corresponding definitions in graminit.h. */
#define Py_single_input 256
#define Py_file_input 257
#define Py_eval_input 258
#define Py_func_type_input 345

/* This doesn't need to match anything */
#define Py_fstring_input 800

If you want to move APIs excluded from the limited C API, you need to create Include/cpython/compile.h and include it from Include/compile.h.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Mar 18, 2021
@bedevere-bot

bedevere-bot commented Mar 18, 2021

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
Move the non-limited C API of include/compile.h into include/cpython.
24f654e
@shihai1991
shihai1991 force-pushed the bpo_35134 branch from 4a2621b to 24f654e Compare March 19, 2021 14:50
@shihai1991 shihai1991 changed the title bpo-35134: Move include/compile.h into include/cpython. bpo-35134: Move non-limited C API of include/compile.h into include/cpython. Mar 19, 2021
vstinner
vstinner reviewed Mar 19, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/C API/2021-03-19-01-26-34.bpo-35134.TMWh1i.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/cpython/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/cpython/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/cpython/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
apply victor's comment
c05f16f
@shihai1991 shihai1991 added the skip news label Mar 19, 2021
vstinner
vstinner reviewed Mar 20, 2021
View reviewed changes
Comment thread Include/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/compile.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
apply victor's comment
6db215a
@vstinner
vstinner merged commit 56f031e into python:master Mar 22, 2021
@bedevere-bot bedevere-bot removed the awaiting changes label Mar 22, 2021
@shihai1991

shihai1991 commented Mar 22, 2021

Copy link
Copy Markdown
Member Author

thanks, victor.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Apr 10, 2022
Add a new Include/cpython/ subdirectory for the "CPython API" with implementation details #79315
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@shihai1991 @bedevere-bot @vstinner @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.