Include what is used in pybind11.h #2499
Conversation
include/pybind11/pybind11.h
Outdated
| #include <memory> | ||
| #include <vector> | ||
| #include <string> | ||
| #include <utility> | ||
|
|
There was a problem hiding this comment.
Before or after the other pybind11 headers? I tend to include stdlib as last includes, but I'm not sure if there's a policy or habit, here. Apart from that, fine with me! :-)
There was a problem hiding this comment.
I can change the order, if you like? :)
There was a problem hiding this comment.
Here: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
I read:
Include headers in the following order: Related header, C system headers, C++ standard library headers, other libraries' headers, your project's headers.
But that is Googles C++ guidelines, not necessarily yours. With reservations, I might have misinterpreted something.. :)
There was a problem hiding this comment.
I think LLVM and therefore the clang-* tools have the same ordering recommendation.
There was a problem hiding this comment.
PS: There is one hard requirement for all Python modules, though: Python.h must precede all stdlib headers.
There was a problem hiding this comment.
Also fine to keep it this way, then. Just wanted to open this for discussion, and check whether this is what we want :-)
There was a problem hiding this comment.
Actually, I think this has to be moved. This puts stdlib includes above <Python.h>, which causes massive numbers of warnings on Python 2.7 on older Linux systems (at least). The rule is Python.h must be above any stdlib includes for any extension module, and that rule is kept by by pybind11.
There was a problem hiding this comment.
Ok! I will do another commit soon, within the hour, moving them under the project headers!
|
Why does this break appveyor? AFAICT, it's working on other branches, but not this, twice now? |
|
Interesting. I thought standard C++ library headers were safe to include anywhere. I have no experience of your pipeline, curious to know why it breaks. Edit: now I know, will commit a reordering as soon as I get home to my machine. |
It's a requirement listed in the CPython docs: https://docs.python.org/3.8/c-api/intro.html#include-files The easiest way I know of to trigger it doing something bad is to use CentOS 7 and the built-in Python 2. You get massive numbers of mismatched redefine warnings if you don't put Python.h first. |
|
Let's see if it fails, still. |
|
Looks like it went well 👍 |
|
Thanks, @Ricardicus! :-) |
|
Thank you! |
3 participants
Adding some includes that the file 'include/pybind11/pybind11.h' is depending on, but not including itself.
Add #include for unique_ptr<> (include/pybind11/pybind11.h:1132)
Add #include for vector<> (include/pybind11/pybind11.h:1735)
Add #include for string (include/pybind11/pybind11.h:2139)
Add #include for move (include/pybind11/pybind11.h:2197)