FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-43244: Rename pycore_ast.h functions to _PyAST_xxx() by vstinner · Pull Request #25252 · python/cpython · GitHub

Repository navigation

bpo-43244: Rename pycore_ast.h functions to _PyAST_xxx() - #25252

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:pyast_funcs
Apr 7, 2021
Merged

vstinner merged 1 commit into
python:masterfrom
vstinner:pyast_funcs

Conversation

vstinner commented Apr 7, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Rename AST functions of pycore_ast.h to use the "PyAST" prefix.
Remove macros creating aliases without prefix. For example, Module()
becomes _PyAST_Module(). Update Grammar/python.gram to use
_PyAST_xxx() functions.

https://bugs.python.org/issue43244

Rename AST functions of pycore_ast.h to use the "_PyAST_" prefix.
Remove macros creating aliases without prefix. For example, Module()
becomes _PyAST_Module(). Update Grammar/python.gram to use
_PyAST_xxx() functions.

vstinner commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

@pablogsal: Does this change look reasonable to you? I already pushed the commit d36d6a9 to prepare the code base (add "Py" prefix in Python-ast.c and ast_opt.c).

My intent is to not pollule the _Py_ C namespace with generic names like _Py_Call() and _Py_Tuple() which look too close to PyObject_Call() and PyTuple_New() names.

Copy link
Copy Markdown
Member

This looks reasonable, but one downside of this is that backporting grammar fixes to 3.9 is going to be nontrivial because this will create merge conflicts in the grammar actions :(

vstinner commented Apr 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

backporting grammar fixes to 3.9

Is that a common thing? I count 7 commits modifying Grammar/python.gram since v3.9.0 (October 2020, 6 months ago) in the 3.9 branch.

vstinner commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

cc @lysnikolaou @isidentical who modified Grammar/python.gram in the last 6 months.

Copy link
Copy Markdown
Member

Not a whole lot to add here. It's true that it's not that common, but it happens from time to time. My feeling is that we can live with it, since with the two parsers being there in 3.9 we already have conflicts most (all?) of the time in the generated C file, when backporting fixes, so it already requires some manual. One line more to change wouldn't make that big of a difference.

pablogsal commented Apr 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

backporting grammar fixes to 3.9

Is that a common thing? I count 7 commits modifying Grammar/python.gram since v3.9.0 (October 2020, 6 months ago) in the 3.9 branch.

We have made some precautions to make backporting easier in the past because the merge conflicts can be a bit nasty but as @lysnikolaou says, we could live with it.

vstinner commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

An alternative would be to create a header file full of #define xxx _PyAST_xxx (one per symbol) and only use it in the parser. Other files already access directly _PyAST_xxx() symbols (currently _Py_xxx()). So you can use directly Module() rather than _Py_Module or _PyAST_Module. Right now, Grammar/python.gram already refer to _Py_xxx() symbol.

vstinner commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

An alternative is to only remove aliases and always use _Py_ prefix on function names: I created #25256 for that. I changes less lines in Grammar/python.gram.

It was my first attempt, but I though that if we are going to change things anyway, it can be an opportunity to "fix" the prefix from _Py_ to _PyAST_.

Copy link
Copy Markdown
Member

Nah, I don't think is worth it. I am fine with this PR

vstinner commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

@pablogsal @lysnikolaou: Since you are the one backporting grammar/parser/compiler fixes, I would prefer if one of you could approve officially the PR. I'm also fine with the status quo (keep aliases), it's more up to you ;-)

vstinner merged commit d27f8d2 into python:master Apr 7, 2021
vstinner deleted the pyast_funcs branch April 7, 2021 19:34

vstinner commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

Thanks for the review @pablogsal and @lysnikolaou! I merged my PR.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL