-
Notifications
You must be signed in to change notification settings - Fork 2.2k
chore: collect all #define PYBIND11_HAS_...
macros in pybind11/detail/common.h
#5647
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
Conversation
@XuehaiPan for a second set of eyes |
I think you can just click "request Copilot review" and Copilot basically generates this, only as a followup comment. |
There was a problem hiding this 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 consolidates all feature macros starting with PYBIND11_HAS_ into a single header (pybind11/detail/common.h) to simplify user compatibility checks and improve internal header inclusion order. Key changes include:
- Removing separate definitions of PYBIND11_HAS_* macros from various files and centralizing them in common.h.
- Updating filesystem support in both tests and header code to rely on the consolidated filesystem macros.
- Eliminating legacy or redundant macros such as PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL and PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/test_stl.cpp | Removed legacy filesystem optional macro and updated include guard logic. |
include/pybind11/stl/filesystem.h | Refactored conditional filesystem includes to use centralized macros. |
include/pybind11/functional.h | Removed standalone PYBIND11_HAS_TYPE_CASTER_STD_FUNCTION_SPECIALIZATIONS macro. |
include/pybind11/detail/native_enum_data.h | Removed legacy native enum macro definition. |
include/pybind11/detail/internals.h | Removed backward compatibility macro for smart holder support. |
include/pybind11/detail/cpp_conduit.h | Removed standalone cpp conduit macro definition. |
include/pybind11/detail/common.h | Centralized all feature macros and updated conditions for newer standards. |
&& !defined(PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL) | ||
# error \ | ||
"Neither #include <filesystem> nor #include <experimental/filesystem is available. (Use -DPYBIND11_HAS_FILESYSTEM_IS_OPTIONAL to ignore.)" | ||
#if defined(PYBIND11_HAS_FILESYSTEM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of the PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL branch changes the behavior for environments where neither nor <experimental/filesystem> is available. Consider updating the documentation or migration notes to inform users of this breaking change.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reasoned this out with the help of Chat:
https://chatgpt.com/share/681da436-a5fc-8008-8d13-402e8bcbaa8e
✅ Conclusion
You're preserving — and even clarifying — expected behavior by:
Removing a test-only macro,
Consolidating detection macros in a single header,
Ensuring that #include <pybind11/stl/filesystem.h>
fails clearly if the compiler lacks support.
So: you’re not overlooking anything, and the PR is correct as is.
Thanks for the reviews @henryiii and @XuehaiPan! |
#define PYBIND11_HAS_...
macros in pybind11/detail/common.h#define PYBIND11_HAS_...
macros in pybind11/detail/common.h
Description
The trigger for working on this PR was seeing complicated code like this.
With the help of my best friend Chat Geepeetee:
This PR consolidates all
PYBIND11_HAS_...
feature macros intopybind11/detail/common.h
.Benefits:
Simplifies user-side compatibility checks:
#ifdef
guards onPYBIND11_HAS_...
macros without needing to worry about which pybind11 headers have been included.Enables safer header inclusion patterns:
Improves internal maintainability:
PYBIND11_HAS_...
macro is visible.This change promotes cleaner and more robust code, both for users and within pybind11's internals.
Minor details:
PYBIND11_HAS_...
macros are now consistently defined as1
.PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
macro is no longer needed (see changes ininclude/pybind11/stl/filesystem.h
andtests/test_stl.cpp
).Before this PR:
With this PR:
(no output)
Suggested changelog entry: