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
graminit.h defines very generic names like 'stmt' or 'test' #79378
Comments
graminit.h is an header file associating Grammar entities to their values, example: #define stmt 269 Problem: defines symbols have no prefix and cause compilation issues on regular C code depending where graminit.h is included. Example with ast.c: static int
validate_stmt(stmt_ty stmt)
{ ... } "#define stmt 269" causes a compilation issue, the preprocessor replaces the code with: static int
validate_stmt(stmt_ty 269)
{ ... } ... which is invalid. Another example:
The preprocessor replaces "exp->v.IfExp.test" with "exp->v.IfExp.305" which is invalid... The compile.h header file works around the issue by redefining 3 constants but using "Py_" prefix: /* These definitions must match corresponding definitions in graminit.h.
There's code in compile.c that checks that they are the same. */
#define Py_single_input 256
#define Py_file_input 257
#define Py_eval_input 258 For comparison, graminit.h uses: #define single_input 256
#define file_input 257
#define eval_input 258 There are different solutions:
Note: "static const int single_input = 305;" cause a complation error on "case single_input": "case label does not reduce to an integer constant". IMHO adding a prefix would be a nice enhancement and a good compromise. It only impacts the four C files which include graminit.h: future.c, ast.c, parsetok.c and parsermodule.c. |
Ah! A friend gave me another solution, define an enum. Example: enum { |
Current workaround in ast.c: /* This is done here, so defines like "test" don't interfere with AST use above. */
#include "grammar.h"
#include "parsetok.h"
#include "graminit.h" This code is in the "middle" of ast.c. |
I think it would be better to exclude graminit.h from the set of distributed headers. It is internal generated file, it should not be used by any user code. Some constants are duplicated as a part of public API in compile.h. |
graminit.h is included in few files, and not always its inclusion is necessary. Here is a patch that minimizes its inclusion. It is not ready for merging, it is just a demo. |
I'm fine with moving it to Include/internal/. Do you know if it's used outside CPython? |
Although not a complete view, all of the mentions I can find in GitHub is in copies of CPython/forks/vendoring of Cpython: |
It can be used by the code that works directly with CST instead of AST: these constants are used as values of the n_type field of the node structure. |
Sorry, I don't know this part of CPython. Is the CST exposed in any way outside CPython internals? If no, I see no reason to expose graminit.h in the public C API. |
Another common issue with Python-ast.h, I just saw it on Windows: compile.c |
Issue fixed in bpo-43244. I moved the evil public Python-ast.h header file into the internal C API as Include/internal/pycore_ast.h. It doesn't define "Yield" anymore: "_PyAST_Yield" must be used instead. |
Moreover, graminit.h was removed in Python 3.10. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: