classification
Title: graminit.h defines very generic names like 'stmt' or 'test'
Type: Stage:
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: pablogsal, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2018-11-09 14:06 by vstinner, last changed 2019-05-14 23:05 by vstinner.

Files
File name Uploaded Description Edit
include-graminit-h.diff serhiy.storchaka, 2018-11-09 15:43
Messages (10)
msg329517 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-09 14:06
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:

        return validate_expr(exp->v.IfExp.test, Load) &&
            validate_expr(exp->v.IfExp.body, Load) &&
            validate_expr(exp->v.IfExp.orelse, Load);

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:

* Do nothing: require to include graminit.h at the right place
* Add a prefix to all symbols, ex: test => Py_test, grammar_test, etc.
* Rename problematic names like 'test' and 'stmt'
* Fix C code to avoid problematic name: 'test' is an an attribute name of a node struct...

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.
msg329518 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-09 14:08
> Note: "static const int single_input = 305;" cause a complation error on "case single_input": "case label does not reduce to an integer constant".

Ah! A friend gave me another solution, define an enum. Example:

enum {
    single_input = 256,
    ...
};
msg329519 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-09 14:10
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.
msg329524 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-09 15:39
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.
msg329526 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-09 15:43
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.
msg329528 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-09 15:54
> 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.

I'm fine with moving it to Include/internal/. Do you know if it's used outside CPython?
msg329533 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2018-11-09 16:37
>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:

https://github.com/search?l=C&p=13&q=graminit.h&type=Code
msg329542 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-09 17:39
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.
msg329571 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-09 23:50
> It can be used by the code that works directly with CST instead of AST: (...)

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.
msg342523 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-14 23:05
Another common issue with Python-ast.h, I just saw it on Windows:

  compile.c 
c:\program files (x86)\windows kits\10\include\10.0.17134.0\um\winbase.h(102): warning C4005: 'Yield': macro redefinition [C:\vstinner\python\master\PCbuild\pythoncore 
.vcxproj]
  c:\vstinner\python\master\include\python-ast.h(627): note: see previous definition of 'Yield'
History
Date User Action Args
2019-05-14 23:05:52vstinnersetmessages: + msg342523
2018-11-09 23:50:24vstinnersetmessages: + msg329571
2018-11-09 17:39:39serhiy.storchakasetmessages: + msg329542
2018-11-09 16:37:37pablogsalsetmessages: + msg329533
2018-11-09 15:54:47vstinnersetmessages: + msg329528
2018-11-09 15:43:02serhiy.storchakasetfiles: + include-graminit-h.diff
keywords: + patch
messages: + msg329526
2018-11-09 15:39:42serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg329524
2018-11-09 14:41:27pablogsalsetnosy: + pablogsal
2018-11-09 14:10:35vstinnersetmessages: + msg329519
2018-11-09 14:08:58vstinnersetmessages: + msg329518
2018-11-09 14:06:44vstinnercreate