Skip to content

bpo-35081: Enhance Python-ast.h and ast.h#10277

Closed
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:pythonasth
Closed

bpo-35081: Enhance Python-ast.h and ast.h#10277
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:pythonasth

Conversation

@vstinner

@vstinner vstinner commented Nov 1, 2018

Copy link
Copy Markdown
Member

Modify asdl_c.py to enhance Python-ast.h:

  • Add #ifndef/#define PYTHON_AST_H to be able to include the header
    twice
  • Add "extern { ... }" for C++
  • Undefine "Yield" macro conflicting with winbase.h
  • ast.h: add node.h and Python-ast.h includes

https://bugs.python.org/issue35081

Modify asdl_c.py to enhance Python-ast.h:

* Add #ifndef/#define PYTHON_AST_H to be able to include the header
  twice
* Add "extern { ... }" for C++
* Undefine "Yield" macro conflicting with winbase.h
* ast.h: add node.h and Python-ast.h includes
@serhiy-storchaka

serhiy-storchaka commented Nov 1, 2018

Copy link
Copy Markdown
Member

What problem does it solve? I have not found anything on bpo-35081, may be open a new issue for discussing?

@vstinner

vstinner commented Nov 1, 2018

Copy link
Copy Markdown
Member Author

What problem does it solve?

I wanted to reorder includes in pylifecycle.c, but ast.h doesn't declare properly its dependencies: it uses node which comes from node.h, and mod_ty which comes from Python-ast.h. This PR fix this.

Undefining the "Yield" macro fix a warning on Windows. In practice, it's more a cleanup.

I have not found anything on bpo-35081, may be open a new issue for discussing?

Well, it's more a "cleanup" change, so I didn't want to open a new issue just for that.

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but a comment should be added to bpo-35081 noting the change to make ast.h easier to include correctly.

Comment thread Include/Python-ast.h
#ifndef PYTHON_AST_H
#define PYTHON_AST_H
#ifdef __cplusplus
extern "C" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this? This is an internal generated header, it is not purposed for direct use.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it's needed if ast.h is included in file compiled in C++. I know that some people do that. At least, it doesn't hurt to add extern "C" { ... }.

Comment thread Include/ast.h
extern "C" {
#endif

#include "node.h" /* node */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other option is to add forward declarations, as in compile.h.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, maybe, but it seems like node.h is included anyway in all C files including ast.h. I don't see the benefit of avoiding to include node.h here. I prefer to be explicit and link ast.h to node.h.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#include "Python-ast.h" is already surrounded with extern "C" { in ast.h.

@vstinner

vstinner commented Nov 6, 2018

Copy link
Copy Markdown
Member Author

Superseded by https://bugs.python.org/issue35177 and PR #10361.

@vstinner vstinner closed this Nov 6, 2018
@vstinner vstinner deleted the pythonasth branch November 6, 2018 14:10
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