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

Open pybind11 namespace with consistent visility. #4098

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

thomaseding
Copy link
Contributor

Description

Pybind opens the pybind11 namespace in its library code via PYBIND11_NAMESPACE to capture visibility settings. The documentation does not open the pybind11 namespace the same way, This implies that forward declaring some pybind11 classes can be done without PYBIND11_NAMESPACE, but this is incorrect in the general case. Ultimately this leads to visibility mismatches depending on definition and include order.

Suggested changelog entry:

Users should use `PYBIND11_NAMESPACE` instead of using `pybind11` when opening namespaces. Using namespace declarations and namespace qualification remain the same as `pybind11`. This is done to ensure consistent symbol visibility.

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.

Thanks Thomas, this looks good to me.

From a small experiment under PR #4050 (merged into the smart_holder branch) I can confirm that this matters on some platforms (e.g. macOS while not using -fvisibility=hidden).

On a related note, I have a PR that shows that -fvisibility=hidden can be removed without losing any pybind11 features (#4072). When working on that PR I overlooked the changes this PR makes to the tests. I'm thinking that will never really matter, because nobody will run them with a mix of pybind11 versions, does that sound right? But regardless, being consistent is better, because test code is copied quite often as a starting point for something new.

@rwgk rwgk requested review from henryiii and Skylion007 July 30, 2022 04:18
@rwgk rwgk merged commit f8e8403 into pybind:master Aug 1, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 1, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022
bmerry added a commit to ska-sa/spead2 that referenced this pull request Jan 26, 2023
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.

4 participants