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-43244: Rename pycore_ast.h functions to _PyAST_xxx() #25252

Merged
merged 1 commit into from
Apr 7, 2021
Merged

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

merged 1 commit into from
Apr 7, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 7, 2021

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

vstinner commented Apr 7, 2021

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

@pablogsal
Copy link
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
Copy link
Member Author

vstinner commented Apr 7, 2021

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

vstinner commented Apr 7, 2021

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

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

pablogsal commented Apr 7, 2021

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

vstinner commented Apr 7, 2021

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

vstinner commented Apr 7, 2021

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

@pablogsal
Copy link
Member

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

@vstinner
Copy link
Member Author

vstinner commented Apr 7, 2021

@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 vstinner merged commit d27f8d2 into python:master Apr 7, 2021
@vstinner vstinner deleted the pyast_funcs branch April 7, 2021 19:34
@vstinner
Copy link
Member Author

vstinner commented Apr 7, 2021

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

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