Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9412 --- Ship it! Update the commit message with a link to the python manual? - Jason Lowe-Power On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
> On Jan. 27, 2017, 3 p.m., Jason Lowe-Power wrote: > > 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 wrote: > 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 wrote: > 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 wrote: > 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 wrote: > 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. 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. - Andreas --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9331 --- On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
> On Jan. 27, 2017, 3 p.m., Jason Lowe-Power wrote: > > 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 wrote: > 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 wrote: > 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 wrote: > 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? 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. - Jason --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9331 --- On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
> On Jan. 27, 2017, 3 p.m., Jason Lowe-Power wrote: > > 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 wrote: > 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 wrote: > 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. 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? - Andreas --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9331 --- On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
> On Jan. 27, 2017, 3 p.m., Jason Lowe-Power wrote: > > 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 wrote: > 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. 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. - Jason --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9331 --- On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
> On Jan. 27, 2017, 3 p.m., Jason Lowe-Power wrote: > > 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. 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. - Andreas --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9331 --- On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9332 --- Ship it! Ship It! - Pierre-Yves Péneau On Jan. 27, 2017, 2:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 2:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/#review9331 --- 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. - Jason Lowe-Power On Jan. 27, 2017, 1:39 p.m., Andreas Sandberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3794/ > --- > > (Updated Jan. 27, 2017, 1:39 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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 > Reviewed-by: Nikos Nikoleris > > > Diffs > - > > src/python/swig/pyevent.cc be62996c95d1 > src/sim/init.cc be62996c95d1 > src/sim/py_interact.cc be62996c95d1 > util/style/sort_includes.py be62996c95d1 > > Diff: http://reviews.gem5.org/r/3794/diff/ > > > Testing > --- > > > Thanks, > > Andreas Sandberg > > ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
[gem5-dev] Review Request 3794: style: Force Python.h to be included before main header
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3794/ --- Review request for Default. Repository: gem5 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 Reviewed-by: Nikos Nikoleris Diffs - src/python/swig/pyevent.cc be62996c95d1 src/sim/init.cc be62996c95d1 src/sim/py_interact.cc be62996c95d1 util/style/sort_includes.py be62996c95d1 Diff: http://reviews.gem5.org/r/3794/diff/ Testing --- Thanks, Andreas Sandberg ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
