Skip to content

Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden" #4072

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.

Sign up for GitHub

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 1 commit into
base: master
Choose a base branch
from
Open

Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden" #4072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@
// on the main `pybind11` namespace.
#if !defined(PYBIND11_NAMESPACE)
# ifdef __GNUG__
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit VISIBILITY sounds like what is being changed, but this doesn't provide an option to choose.

Consider using HIDDEN to make intent more clear.

# define PYBIND11_NAMESPACE pybind11 __attribute__((visibility("hidden")))
# define PYBIND11_NS_VISIBILITY(...) __VA_ARGS__ __attribute__((visibility("hidden")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit While NS is a good abbreviation, perhaps it is still good to full expand NAMESPACE?

Or if this is agnostic to the type of symbol, consider just removing the NS qualifier?

# define PYBIND11_NAMESPACE PYBIND11_NS_VISIBILITY(pybind11)
# else
# define PYBIND11_NS_VISIBILITY(...) __VA_ARGS__
# define PYBIND11_NAMESPACE pybind11
# endif
#endif
Expand Down
1 change: 0 additions & 1 deletion pybind11/setup_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
if WIN:
cflags += ["/EHsc", "/bigobj"]
else:
cflags += ["-fvisibility=hidden"]
env_cflags = os.environ.get("CFLAGS", "")
env_cppflags = os.environ.get("CPPFLAGS", "")
c_cpp_flags = shlex.split(env_cflags) + shlex.split(env_cppflags)
Expand Down
2 changes: 1 addition & 1 deletion tests/pybind11_cross_module_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) {
if (p) {
std::rethrow_exception(p);
}
} catch (const shared_exception &e) {
} catch (const pybind11_tests::shared_exception &e) {
PyErr_SetString(PyExc_KeyError, e.what());
}
});
Expand Down
5 changes: 5 additions & 0 deletions tests/test_custom_type_setup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@

namespace py = pybind11;

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I understand need for applying hidden when it only contains an anonymous namespace.

Is it possible to revert this (possibly redundant) change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Eric, I'll wait for Henry's opinion before investing more time, but to explain quickly, this is purely to appease some sad compilers that don't "realize" that the unnamed namespace is already hidden. I could add comments, or remove the unnamed namespace, I'll figure out something after we decided the big question.

namespace {

struct OwnsPythonObjects {
py::object value = py::none();
};

} // namespace
PYBIND11_NAMESPACE_END(pybind11_tests)

TEST_SUBMODULE(custom_type_setup, m) {
using namespace pybind11_tests;

py::class_<OwnsPythonObjects> cls(
m, "OwnsPythonObjects", py::custom_type_setup([](PyHeapTypeObject *heap_type) {
auto *type = &heap_type->ht_type;
Expand Down
6 changes: 6 additions & 0 deletions tests/test_exceptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include <stdexcept>
#include <utility>

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - why not just use anonymous namespace {}?


// A type that should be raised as an exception in Python
class MyException : public std::exception {
public:
Expand Down Expand Up @@ -110,7 +112,11 @@ std::string error_already_set_what(const py::object &exc_type, const py::object
return py::error_already_set().what();
}

PYBIND11_NAMESPACE_END(pybind11_tests)

TEST_SUBMODULE(exceptions, m) {
using namespace pybind11_tests;

m.def("throw_std_exception",
[]() { throw std::runtime_error("This exception was intentionally thrown."); });

Expand Down
4 changes: 4 additions & 0 deletions tests/test_exceptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@

// shared exceptions for cross_module_tests

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - why not just use anonymous namespace {}?


class PYBIND11_EXPORT_EXCEPTION shared_exception : public pybind11::builtin_exception {
public:
using builtin_exception::builtin_exception;
explicit shared_exception() : shared_exception("") {}
void set_error() const override { PyErr_SetString(PyExc_RuntimeError, what()); }
};

PYBIND11_NAMESPACE_END(pybind11_tests)
6 changes: 6 additions & 0 deletions tests/test_numpy_array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include <cstdint>
#include <utility>

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - why not just use anonymous namespace {}?


// Size / dtype checks.
struct DtypeCheck {
py::dtype numpy{};
Expand Down Expand Up @@ -159,7 +161,11 @@ py::handle auxiliaries(T &&r, T2 &&r2) {
// note: declaration at local scope would create a dangling reference!
static int data_i = 42;

PYBIND11_NAMESPACE_END(pybind11_tests)

TEST_SUBMODULE(numpy_array, sm) {
using namespace pybind11_tests;

try {
py::module_::import("numpy");
} catch (const py::error_already_set &) {
Expand Down
4 changes: 2 additions & 2 deletions tests/test_pytypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

#include <utility>

namespace external {
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(external))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit Er, hiding a namespace external smells odd.

Can we use a better name here? e.g. feaux_external, with a comment of what is going on?

namespace detail {
bool check(PyObject *o) { return PyFloat_Check(o) != 0; }

Expand All @@ -37,7 +37,7 @@ class float_ : public py::object {

double get_value() const { return PyFloat_AsDouble(this->ptr()); }
};
} // namespace external
PYBIND11_NAMESPACE_END(external)

namespace implicit_conversion_from_0_to_handle {
// Uncomment to trigger compiler error. Note: Before PR #4008 this used to compile successfully.
Expand Down
13 changes: 0 additions & 13 deletions tools/pybind11NewTools.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,19 +203,6 @@ function(pybind11_add_module target_name)
target_link_libraries(${target_name} PRIVATE pybind11::windows_extras)
endif()

# -fvisibility=hidden is required to allow multiple modules compiled against
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW yay! deleting code makes me 10000% happier!

# different pybind versions to work properly, and for some features (e.g.
# py::module_local). We force it on everything inside the `pybind11`
# namespace; also turning it on for a pybind module compilation here avoids
# potential warnings or issues from having mixed hidden/non-hidden types.
if(NOT DEFINED CMAKE_CXX_VISIBILITY_PRESET)
set_target_properties(${target_name} PROPERTIES CXX_VISIBILITY_PRESET "hidden")
endif()

if(NOT DEFINED CMAKE_CUDA_VISIBILITY_PRESET)
set_target_properties(${target_name} PROPERTIES CUDA_VISIBILITY_PRESET "hidden")
endif()

# If we don't pass a WITH_SOABI or WITHOUT_SOABI, use our own default handling of extensions
if(NOT ARG_WITHOUT_SOABI AND NOT "WITH_SOABI" IN_LIST ARG_UNPARSED_ARGUMENTS)
pybind11_extension(${target_name})
Expand Down
13 changes: 0 additions & 13 deletions tools/pybind11Tools.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,19 +183,6 @@ function(pybind11_add_module target_name)

pybind11_extension(${target_name})

# -fvisibility=hidden is required to allow multiple modules compiled against
# different pybind versions to work properly, and for some features (e.g.
# py::module_local). We force it on everything inside the `pybind11`
# namespace; also turning it on for a pybind module compilation here avoids
# potential warnings or issues from having mixed hidden/non-hidden types.
if(NOT DEFINED CMAKE_CXX_VISIBILITY_PRESET)
set_target_properties(${target_name} PROPERTIES CXX_VISIBILITY_PRESET "hidden")
endif()

if(NOT DEFINED CMAKE_CUDA_VISIBILITY_PRESET)
set_target_properties(${target_name} PROPERTIES CUDA_VISIBILITY_PRESET "hidden")
endif()

if(ARG_NO_EXTRAS)
return()
endif()
Expand Down