Skip to content

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:

CI testing now includes ``-Wwrite-strings -Wunreachable-code -Wpointer-arith -Wredundant-decls`` in some jobs.

@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.

#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
@rhaschke rhaschke force-pushed the fix-redundant-declarations branch from afb5f38 to a77a7c3 Compare April 29, 2025 13:49
@rhaschke
Copy link
Contributor Author

This should be ready for review. The failing test seems to be unrelated to my changes.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

Waiting a bit for @henryiii to take a look.

// return -1; // Unreachable.
}

// The implementation needs the definition of `class cpp_function`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for catching this!

@rwgk rwgk merged commit c7f3460 into pybind:master Apr 30, 2025
112 of 113 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 30, 2025
@rwgk
Copy link
Collaborator

rwgk commented Apr 30, 2025

@rhaschke I made a quick change to the Suggested changelog entry. Could you please review, and maybe make it more precise and correct?

b-pass pushed a commit to b-pass/pybind11 that referenced this pull request May 9, 2025
* CI: Fail on any warnings with clang 18

* CI: Fail on any warnings with gcc 13

* Fix cmake's try_compile warning

* Guard redundant declarations

* ci.yml: fix syntax error

* Use PYBIND11_WARNING macros

* Fix more redundant declarations

... introduced with merge of smart_holder branch
@henryiii henryiii changed the title Fix gcc compiler warnings ci: fix gcc compiler warnings May 17, 2025
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label May 17, 2025
@rhaschke rhaschke deleted the fix-redundant-declarations branch August 8, 2025 08:05
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