Skip to content

Open pybind11 namespace with consistent visility. #4098

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants