style: Force Python.h to be included before main header

Information
Submitter:	Andreas Sandberg
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11803:1dba09aeeefd
---------------------------
style: Force Python.h to be included before main header

Python's header files set various compiler macros (e.g.,
_XOPEN_SOURCE) unconditionally. This triggers preprocessor warnings
that end up being treated as errors. The style guide used to mandate
that Python headers are included before any other header. This
requirement was changed to always include a source file's main header
first, which ended up triggering these errors.

This change updates the style checker to always include Python.h
before the main header file.

Change-Id: Id6a4f7fc64a336a8fd26691a0ca682abeb1d1579
Signed-off-by: Andreas Sandberg <andreas.sandberg@arm.com>
Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Testing Done

Oops. I should have included this in my description of http://reviews.gem5.org/r/3792/. I think another solution is to include Python.h in src/python/swig/pyevent.hh. See my change in http://reviews.gem5.org/r/3792/diff/1/#33.

If my fix does work, I think it's better to just include Python.h there instead of changing the style.

Andreas Sandberg Jan. 27, 2017, 8:59 a.m. (Jan. 27, 2017, 8:59 a.m.)

I'd like to avoid including Python.h in any header files for dependency reasons. Another reason for including it before any other header is that it solves this entire class of issues (the flip side is that we wouldn't always spot headers that requires it).

We actually used to require the Python header to be first in the files that required it, but it got lost in a a style update a while back.

Jason Lowe-Power Jan. 27, 2017, 9:04 a.m. (Jan. 27, 2017, 9:04 a.m.)

Could you explain more about why you don't want to include Python.h in pyevent.hh? It seems to me that since this file requires things declared in Python.h (e.g., PyObject) that it should be included here. Otherwise, we have to remember that anytime we include python/swig/pyevent.hh we first must include Python.h.

Andreas Sandberg Jan. 27, 2017, 9:07 a.m. (Jan. 27, 2017, 9:07 a.m.)

Sorry, you're absolutely right. It should definitely be included in pyevent.hh. Does that solve the rest of the Python issues? If so, do we need/want this change?

Jason Lowe-Power Jan. 27, 2017, 9:10 a.m. (Jan. 27, 2017, 9:10 a.m.)

I think it solves all of the issues. I was able to use http://reviews.gem5.org/r/3792/ and http://reviews.gem5.org/r/3779/ to resolve all of the build issues on Arch Linux.

I'm not opposed to pushing this to change the style back, if we think that makes sense, too. But I think it's somewhat orthogonal to the build errors.

Andreas Sandberg Feb. 7, 2017, 7:04 a.m. (Feb. 7, 2017, 7:04 a.m.)

I had a look at the Python manual, this is what it states:

Note Since Python may define some pre-processor definitions which affect the standard headers on some systems, you must include Python.h before any standard headers are included.

Seems like we should include Python.h first after all.

Ship It!

Update the commit message with a link to the python manual?

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Status: Closed (submitted)