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

Fix gcc compiler warnings #5523

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Feb 15, 2025

Description

Removed redundant declarations causing the following warnings (including pybind11.h):

In file included from pybind11/include/pybind11/cast.h:15,
                 from pybind11/include/pybind11/attr.h:14,
                 from pybind11/include/pybind11/detail/class.h:12,
                 from pybind11/include/pybind11/pybind11.h:12,
                 from include/py_binding_tools/ros_msg_typecasters.h:39,
                 from src/ros_msg_typecasters.cpp:37:
pybind11/include/pybind11/detail/type_caster_base.h:521:13: warning: redundant redeclaration of ‘bool pybind11::detail::deregister_instance(pybind11::detail::instance*, void*, const pybind11::detail::type_info*)’ in same scope [-Wredundant-decls]
  521 | inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
      |             ^~~~~~~~~~~~~~~~~~~
In file included from pybind11/include/pybind11/detail/type_caster_base.h:14,
                 from pybind11/include/pybind11/cast.h:15,
                 from pybind11/include/pybind11/attr.h:14,
                 from pybind11/include/pybind11/detail/class.h:12,
                 from pybind11/include/pybind11/pybind11.h:12,
                 from include/py_binding_tools/ros_msg_typecasters.h:39,
                 from src/ros_msg_typecasters.cpp:37:
pybind11/include/pybind11/trampoline_self_life_support.h:19:13: note: previous declaration of ‘bool pybind11::detail::deregister_instance(pybind11::detail::instance*, void*, const pybind11::detail::type_info*)’
   19 | inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
      |             ^~~~~~~~~~~~~~~~~~~
In file included from pybind11/include/pybind11/pybind11.h:12,
                 from include/py_binding_tools/ros_msg_typecasters.h:39,
                 from src/ros_msg_typecasters.cpp:37:
pybind11/include/pybind11/detail/class.h:502:13: warning: redundant redeclaration of ‘std::string pybind11::detail::error_string()’ in same scope [-Wredundant-decls]
  502 | std::string error_string();
      |             ^~~~~~~~~~~~
In file included from pybind11/include/pybind11/detail/internals.h:19,
                 from pybind11/include/pybind11/gil.h:17,
                 from pybind11/include/pybind11/detail/type_caster_base.h:12,
                 from pybind11/include/pybind11/cast.h:15,
                 from pybind11/include/pybind11/attr.h:14,
                 from pybind11/include/pybind11/detail/class.h:12,
                 from pybind11/include/pybind11/pybind11.h:12,
                 from include/py_binding_tools/ros_msg_typecasters.h:39,
                 from src/ros_msg_typecasters.cpp:37:
pybind11/include/pybind11/pytypes.h:743:20: note: previous declaration of ‘std::string pybind11::detail::error_string()’
  743 | inline std::string error_string() {
      |                    ^~~~~~~~~~~~

Suggested changelog entry:

Fix gcc compiler warnings

@rhaschke rhaschke force-pushed the fix-redundant-declarations branch from cf9cac3 to b3c6954 Compare February 16, 2025 06:04
@rhaschke rhaschke changed the base branch from smart_holder to master February 16, 2025 06:06
@rhaschke rhaschke changed the title [smart_holder] Fix redundant declarations Fix redundant declarations Feb 16, 2025
@rhaschke rhaschke closed this Feb 16, 2025
@rhaschke rhaschke reopened this Feb 16, 2025
@rhaschke
Copy link
Contributor Author

This affects master branch too, not only smart_holder.

@rhaschke rhaschke marked this pull request as draft February 16, 2025 06:21
@rhaschke
Copy link
Contributor Author

Looks like some tests sparsely use includes. Will check later.

@rhaschke
Copy link
Contributor Author

Let's start over and push commits one-by-one.

@rhaschke rhaschke force-pushed the fix-redundant-declarations branch from b3c6954 to aedf53b Compare February 16, 2025 10:11
@rhaschke
Copy link
Contributor Author

The pypy-3.8 build seems to be flaky.

@rhaschke rhaschke marked this pull request as ready for review February 16, 2025 12:15
@rhaschke rhaschke requested a review from henryiii as a code owner February 16, 2025 12:15
@rhaschke rhaschke changed the title Fix redundant declarations Fix gcc compiler warnings Feb 16, 2025
@rwgk
Copy link
Collaborator

rwgk commented Feb 16, 2025

The pypy-3.8 build seems to be flaky.

Yes, very often.

@henryiii
Copy link
Collaborator

We need to move to pypy-3.10. Older pypy's aren't supported by upstream.

@@ -3,6 +3,7 @@
#pragma once

#include "detail/common.h"
#include "detail/type_caster_base.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I think this is going in the wrong direction:

It brings in a ton of dependencies unrelated to the gil_safe_call_once functionality.

It's seems counter-productive to turn on compiler warnings just to appease them.

A great step forward would be to refactor the code organization. I'd be very happy if you did that, even if several pieces of code need to be moved around.

If you think that's too much work: Simply disable the warnings for the troublesome existing forward declaration. That'd be a net gain: Having the warnings enabled will nudge us in the right direction when working on new code. The warning suppressions will remind us that we have a code organization cleanup job waiting for an intrepid volunteer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. For restructuring the code, I'm not an expert enough. I will disable the warnings. Switching to draft state for now.

@rhaschke rhaschke marked this pull request as draft February 17, 2025 23:58
@rhaschke rhaschke marked this pull request as ready for review February 18, 2025 16:18
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