Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PYBIND11_NOINLINE-related cleanup. #3179

Merged
merged 19 commits into from
Aug 9, 2021
Merged

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 7, 2021

Pure cleanup for correctness and consistency.

Motivated by ongoing work on removing GCC -Wattributes from the pragma block at the top of pybind11.h (see PR #3125 for full context), but technically completely independent and a worthwhile cleanup in its own right.

Correctness aspect:

Consistency aspect:

  • Current master has 4 x inline PYBIND11_NOINLINE and 17 x PYBIND11_NOINLINE inline. See lists below.
  • To remove the potential for such inconsistencies, and to make the code nicer to read, inline is added to the PYBIND11_NOINLINE macro.

Pointing out a special case in this PR (the only special case):

Converted from inline to PYBIND11_NOINLINE, for consistency with an overload:

pybind11.h:inline void keep_alive_impl(handle nurse, handle patient) {

Current master:

inline first:

numpy.h:inline PYBIND11_NOINLINE void load_numpy_internals(numpy_internals* &ptr) {
numpy.h:inline PYBIND11_NOINLINE void register_structured_dtype(
detail/internals.h:inline PYBIND11_NOINLINE void *get_shared_data(const std::string &name) {
detail/internals.h:inline PYBIND11_NOINLINE void *set_shared_data(const std::string &name, void *data) {

inline second:

pybind11.h:PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {
pybind11.h:PYBIND11_NOINLINE inline void print(const tuple &args, const dict &kwargs) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline void all_type_info_populate(PyTypeObject *t, std::vector<type_info *> &bases) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline detail::type_info *get_type_info(const std::type_index &tp,
detail/type_caster_base.h:PYBIND11_NOINLINE inline handle get_type_handle(const std::type_info &tp, bool throw_if_missing) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline handle find_registered_python_instance(void *src,
detail/type_caster_base.h:PYBIND11_NOINLINE inline value_and_holder instance::get_value_and_holder(const type_info *find_type /*= nullptr default in common.h*/, bool throw_if_missing /*= true in common.h*/) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline void instance::allocate_layout() {
detail/type_caster_base.h:PYBIND11_NOINLINE inline void instance::deallocate_layout() const {
detail/type_caster_base.h:PYBIND11_NOINLINE inline bool isinstance_generic(handle obj, const std::type_info &tp) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline std::string error_string() {
detail/type_caster_base.h:PYBIND11_NOINLINE inline handle get_object_handle(const void *ptr, const detail::type_info *type ) {
detail/common.h:[[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const char *reason) { throw std::runtime_error(reason); }
detail/common.h:[[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const std::string &reason) { throw std::runtime_error(reason); }
detail/typeid.h:PYBIND11_NOINLINE inline void clean_type_id(std::string &name) {
detail/internals.h:PYBIND11_NOINLINE inline internals &get_internals() {

@rwgk
Copy link
Collaborator Author

rwgk commented Aug 9, 2021

Oops, I forgot to move this out of Draft ... again. Sorry.

@rwgk rwgk marked this pull request as ready for review August 9, 2021 13:30
@rwgk rwgk force-pushed the noinline_cleanup branch from 73e2192 to 4239aed Compare August 9, 2021 14:58
@rwgk
Copy link
Collaborator Author

rwgk commented Aug 9, 2021

Thanks for the approvals!

For completeness, I just did one more quick validation, only locally on my workstation, by changing this one line back in pytypes.h, while leaving everything else the same as in this PR:

-bool isinstance_generic(handle obj, const std::type_info &tp);
+inline bool isinstance_generic(handle obj, const std::type_info &tp);

With GCC 10.2.1, I'm counting 43 warnings when building all tests under tests/ (i.e. without building the embedding tests). One example below.

That warning is really useful, clang 11 in contrast doesn't seem to have an equivalent warning, at least we're not activating it if it exists. Potential follow-on work item?

Another potential follow-on work item: is there a way to get warnings about inline in forward declarations, even if the implementation is also inline? It's basically just for style, to be consistent. But also to preempt the type of confusion I was in initially, until I finally came across the stackoverflow article.

/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/type_caster_base.h:405:24: warning: declaration of ‘bool pybind11::detail::isinstance_generic(pybind11::handle, const std::type_info&)’ with attribute ‘noinline’ follows inline declaration [-Wattributes]
  405 | PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) {
      |                        ^~~~~~~~~~~~~~~~~~                               
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:13,
                 from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/attr.h:13,
                 from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:18,
                 from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/embed.h:12,
                 from /usr/local/google/home/rwgk/forked/pybind11/tests/test_embed/test_interpreter.cpp:1:
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pytypes.h:27:13: note: previous declaration of ‘bool pybind11::detail::isinstance_generic(pybind11::handle, const std::type_info&)’ here
   27 | inline bool isinstance_generic(handle obj, const std::type_info &tp);   
      |             ^~~~~~~~~~~~~~~~~~                                          
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:16,
                 from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/attr.h:13,
                 from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:18,
                 from /usr/local/google/home/rwgk/forked/pybind11/tests/cross_module_gil_utils.cpp:9:

@rwgk rwgk merged commit 4c7e509 into pybind:master Aug 9, 2021
@rwgk rwgk deleted the noinline_cleanup branch August 9, 2021 17:10
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 9, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants