Skip to content

Remove inline from declarations #5664

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

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

Conversation

bricknerb
Copy link

Description

Function declarations that are not definitions should not be declared inline.
Otherwise, they could trigger undefined-inline warning, which should actually be an error.

@rwgk
Copy link
Collaborator

rwgk commented May 14, 2025

I'm totally on board with this, but had trouble in the past convincing other maintainers.

How did you discover these? Is there a tool or option that we could run in our CI, to make sure we don't have inline declarations?

@henryiii henryiii requested a review from Copilot May 15, 2025 02:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the explicit use of the inline keyword from function declarations that are not definitions, in order to avoid triggering undefined-inline warnings.

  • Removed inline qualifiers from function declarations in type_caster_base.h
  • Updated forward declarations in internals.h
  • Adjusted the extern "C" function declaration in cpp_conduit.h

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
include/pybind11/detail/type_caster_base.h Removed inline from function declarations to avoid undefined-inline warnings.
include/pybind11/detail/internals.h Removed inline from several forward declarations for consistency and clarity.
include/pybind11/detail/cpp_conduit.h Removed inline from the extern "C" function declaration to adhere to declaration standards.
Comments suppressed due to low confidence (7)

include/pybind11/detail/type_caster_base.h:96

  • Removing 'inline' from this declaration is appropriate since it is only a declaration and not a definition; this change helps avoid undefined-inline warnings.
inline std::pair<decltype(internals::registered_types_py)::iterator, bool>

include/pybind11/detail/type_caster_base.h:501

  • Removing 'inline' here is correct because the function is declared rather than defined; this prevents potential undefined-inline issues.
inline PyObject *make_new_instance(PyTypeObject *type);

include/pybind11/detail/internals.h:53

  • Eliminating the inline keyword from this forward declaration conforms to best practices for non-definition declarations.
inline PyTypeObject *make_static_property_type();

include/pybind11/detail/internals.h:54

  • Removing 'inline' from this declaration is appropriate to avoid confusion and unintended inline linkage.
inline PyTypeObject *make_default_metaclass();

include/pybind11/detail/internals.h:55

  • Removing the inline keyword here maintains consistency across forward declarations and prevents undefined-inline issues.
inline PyObject *make_object_base_type(PyTypeObject *metaclass);

include/pybind11/detail/internals.h:56

  • The inline removal is correct for this function declaration, ensuring that only its definition would carry such specifiers if needed.
inline void translate_exception(std::exception_ptr p);

include/pybind11/detail/cpp_conduit.h:15

  • Removing 'inline' from this extern "C" declaration is in line with proper declaration practices and prevents undefined-inline warnings.
extern "C" inline PyObject *pybind11_object_new(PyTypeObject *type, PyObject *, PyObject *);

@bricknerb
Copy link
Author

I'm totally on board with this, but had trouble in the past convincing other maintainers.

How did you discover these? Is there a tool or option that we could run in our CI, to make sure we don't have inline declarations?

I've noticed that this triggers undefined-inline warning in a codebase that uses this.

@rwgk
Copy link
Collaborator

rwgk commented May 15, 2025

I've noticed that this triggers undefined-inline warning in a codebase that uses this.

Could you please provide an example compilation command line, so that we can reproduce, and include this in the CI here?

If we don't do that, these problems will continue to creep up.

The more specific you make it, the better. Guessing information is much harder than reducing.

But probably, all we need is the compiler name and version (e.g. g++ --version output or similar), and one complete compilation command. Maybe also the platform (e.g. Ubuntu), but usually that doesn't matter.

@bricknerb
Copy link
Author

I've noticed that this triggers undefined-inline warning in a codebase that uses this.

Could you please provide an example compilation command line, so that we can reproduce, and include this in the CI here?

If we don't do that, these problems will continue to creep up.

The more specific you make it, the better. Guessing information is much harder than reducing.

But probably, all we need is the compiler name and version (e.g. g++ --version output or similar), and one complete compilation command. Maybe also the platform (e.g. Ubuntu), but usually that doesn't matter.

I believe this will trigger when it's compiled with clang.

@henryiii
Copy link
Collaborator

henryiii commented May 18, 2025

We compile in clang with warnings as errors on. Is there some warning flag needed to get this? We can add that flag. I don't see it here: https://clang.llvm.org/docs/DiagnosticsReference.html

@rwgk
Copy link
Collaborator

rwgk commented May 18, 2025

To add to @henryiii's request, earlier you wrote:

I've noticed that this triggers undefined-inline warning in a codebase that uses this.

If you could just copy-paste what you see, that'll help enormously.

It's fine if you need to redact the pathnames. What we need to know is the compiler version and all command line options.

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