Skip to content

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Nov 1, 2018

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

serhiy-storchaka commented Nov 1, 2018

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

@vstinner
Copy link
Member Author

vstinner commented Nov 1, 2018

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.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

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.

#ifndef PYTHON_AST_H
#define PYTHON_AST_H
#ifdef __cplusplus
extern "C" {
Copy link
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
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" { ... }.

extern "C" {
#endif

#include "node.h" /* node */
Copy link
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
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
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
Copy link
Member Author

vstinner commented Nov 6, 2018

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