Skip to content

Commit

Permalink
Use std::hash<std::type_index>, std::equal_to<std::type_index> ev…
Browse files Browse the repository at this point in the history
…erywhere **except when libc++ is in use** (#4319)

* Try using `std::hash<std::type_index>`, `std::equal_to<std::type_index>` everywhere.

From PR #4316 we know that types in the unnamed namespace in different translation units do not compare equal, as desired.

But do types in named namespaces compare equal, as desired?

* Revert "Try using `std::hash<std::type_index>`, `std::equal_to<std::type_index>` everywhere."

This reverts commit a06949a9265014b3c581396c4f37c45ccc03dea6.

* Use "our own name-based hash and equality functions" for `std::type_index` only under macOS, based on results shown under pybind/pybind11#4316 (comment)

* Patch in PR #4313: Minimal reproducer for clash when binding types defined in the unnamed namespace.

* test_unnamed_namespace_b xfail for clang

* `PYBIND11_INTERNALS_VERSION 5`

* Add a note to docs/classes.rst

* For compatibility with Google-internal testing, test_unnamed_namespace_a & test_unnamed_namespace_b need to work when imported in any order.

* Trying "__GLIBCXX__ or Windows", based on observations from Google-internal testing.

* Try _LIBCPP_VERSION

* Account for libc++ behavior in tests and documentation.

* Adjust expectations for Windows Clang (and make code less redundant).

* Add WindowsClang to ci.yml

Added block transferred from PR #4321

* Add clang-latest to name that appears in the GitHub Actions web view.

* Tweak the note in classes.rst again.

* Add `pip install --upgrade pip`, Show env, cosmetic changes

Already tested under PR #4321

* Add macos_brew_install_llvm to ci.yml

Added block transferred from PR #4324

* `test_cross_module_exception_translator` xfail 'Homebrew Clang'

* Revert back to base version of .github/workflows/ci.yml (the ci.yml changes were merged under #4323 and #4326)

* Fixes for ruff

* Make updated condition in internals.h dependent on ABI version.

* Remove PYBIND11_TEST_OVERRIDE when testing with PYBIND11_INTERNALS_VERSION=10000000

* Selectively exercise cmake `-DPYBIND11_TEST_OVERRIDE`: ubuntu, macos, windows

Extra work added to quick jobs, based on timings below, to not increase the GHA start-to-last-job-finished time.

```
Duration
^              Number of pytest runs
^              ^ Job identifier
^              ^ ^
0:03:48.024227 1 1___3___Clang_3.6___C++11___x64.txt
0:03:58.992814 1 2___3___Clang_3.7___C++11___x64.txt
0:04:25.758942 1 1___3.7___Debian___x86____Install.txt
0:04:50.148276 1 4___3___Clang_7___C++11___x64.txt
0:04:55.784558 1 13___3___Clang_15___C++20___x64.txt
0:04:57.048754 1 6___3___Clang_dev___C++11___x64.txt
0:05:00.485181 1 7___3___Clang_5___C++14___x64.txt
0:05:03.744964 1 2___3___almalinux8___x64.txt
0:05:06.222752 1 5___3___Clang_9___C++11___x64.txt
0:05:11.767022 1 2___3___GCC_7___C++17__x64.txt
0:05:18.634930 1 2___3.11__deadsnakes____x64.txt
0:05:22.810995 1 1___3___GCC_7___C++11__x64.txt
0:05:25.275317 1 12___3___Clang_14___C++20___x64.txt
0:05:32.058174 1 5___3___GCC_10___C++17__x64.txt
0:05:39.381351 1 7___3___GCC_12___C++20__x64.txt
0:05:40.502252 1 8___3___Clang_10___C++17___x64.txt
0:05:59.344905 1 3___3___Clang_3.9___C++11___x64.txt
0:06:10.825147 1 6___3___GCC_11___C++20__x64.txt
0:06:20.655443 1 3___3___almalinux9___x64.txt
0:06:22.472061 1 3___3___GCC_8___C++14__x64.txt
0:06:42.647406 1 11___3___Clang_13___C++20___x64.txt
0:06:53.352720 1 1___3.10___CUDA_11.7___Ubuntu_22.04.txt
0:07:07.357801 1 2___3.7___MSVC_2019___x86_-DCMAKE_CXX_STANDARD=14.txt
0:07:09.057603 1 1___3___centos7___x64.txt
0:07:15.546282 1 1___3.8___MSVC_2019__Debug____x86_-DCMAKE_CXX_STANDARD=17.txt
0:07:22.566022 1 4___3___GCC_8___C++17__x64.txt
0:08:13.592674 1 2___3.9___MSVC_2019__Debug____x86_-DCMAKE_CXX_STANDARD=20.txt
0:08:16.422768 1 9___3___Clang_11___C++20___x64.txt
0:08:21.168457 1 3___3.8___MSVC_2019___x86_-DCMAKE_CXX_STANDARD=17.txt
0:08:27.129468 1 10___3___Clang_12___C++20___x64.txt
0:09:35.045470 1 1___3.10___windows-latest___clang-latest.txt
0:09:57.361843 1 1___3.9___MSVC_2022_C++20___x64.txt
0:10:35.187767 1 1___3.6___MSVC_2019___x86.txt
0:11:14.691200 4 2___3.9___ubuntu-20.04___x64.txt
0:11:37.701167 1 1_macos-latest___brew_install_llvm.txt
0:11:38.688299 4 4___3.11___ubuntu-20.04___x64.txt
0:11:52.720216 1 4___3.9___MSVC_2019___x86_-DCMAKE_CXX_STANDARD=20.txt
0:13:23.456591 4 6___pypy-3.8___ubuntu-20.04___x64_-DPYBIND11_FINDPYTHON=ON.txt
0:13:25.863592 2 1___3___ICC_latest___x64.txt
0:13:32.411758 3 9___3.9___windows-2022___x64.txt
0:13:45.473377 4 3___3.10___ubuntu-20.04___x64.txt
0:13:55.366447 4 5___pypy-3.7___ubuntu-20.04___x64.txt
0:13:57.969502 3 10___3.10___windows-2022___x64.txt
0:14:19.837475 3 11___3.11___windows-2022___x64.txt
0:14:33.316770 4 1___3.6___ubuntu-20.04___x64_-DPYBIND11_FINDPYTHON=ON_-DCMA.txt
0:15:34.449278 4 22___3.6___windows-2019___x64_-DPYBIND11_FINDPYTHON=ON.txt
0:16:25.189055 2 1___3.9-dbg__deadsnakes____Valgrind___x64.txt
0:17:20.956667 4 15___3.6___macos-latest___x64.txt
0:17:27.513891 4 23___3.9___windows-2019___x64.txt
0:17:58.783286 3 8___3.6___windows-2022___x64.txt
0:18:25.917828 4 7___pypy-3.9___ubuntu-20.04___x64.txt
0:19:17.399820 3 13___pypy-3.8___windows-2022___x64.txt
0:19:45.002122 3 12___pypy-3.7___windows-2022___x64.txt
0:20:03.201926 4 16___3.9___macos-latest___x64.txt
0:20:15.415178 4 17___3.10___macos-latest___x64.txt
0:20:20.263216 4 20___pypy-3.8___macos-latest___x64.txt
0:20:31.998226 3 1___3___windows-latest___mingw64.txt
0:20:40.812286 4 18___3.11___macos-latest___x64.txt
0:22:47.714749 4 19___pypy-3.7___macos-latest___x64.txt
0:23:04.435859 3 2___3___windows-latest___mingw32.txt
0:25:48.719597 3 14___pypy-3.9___windows-2022___x64.txt
0:26:01.211688 4 21___pypy-3.9___macos-latest___x64.txt
0:28:19.971015 1 1___3___CentOS7__PGI_22.9___x64.txt
```

* Update skipif for Python 3.12a7 (the WIP needs to be handled in a separate PR).
  • Loading branch information
Ralf W. Grosse-Kunstleve authored Apr 25, 2023
1 parent ff7f5df commit 6de6191
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 3 deletions.
1 change: 0 additions & 1 deletion .github/workflows/upstream.yml
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ jobs:
-DDOWNLOAD_EIGEN=ON
-DCMAKE_CXX_STANDARD=17
-DPYBIND11_INTERNALS_VERSION=10000000
"-DPYBIND11_TEST_OVERRIDE=test_call_policies.cpp;test_gil_scoped.cpp;test_thread.cpp"
- name: Build (unstable ABI)
run: cmake --build build17max -j 2
Expand Down
10 changes: 10 additions & 0 deletions docs/classes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ interactive Python session demonstrating this example is shown below:
Static member functions can be bound in the same way using
:func:`class_::def_static`.

.. note::

Binding C++ types in unnamed namespaces (also known as anonymous namespaces)
works reliably on many platforms, but not all. The `XFAIL_CONDITION` in
tests/test_unnamed_namespace_a.py encodes the currently known conditions.
For background see `#4319 <https://github.com/pybind/pybind11/pull/4319>`_.
If portability is a concern, it is therefore not recommended to bind C++
types in unnamed namespaces. It will be safest to manually pick unique
namespace names.

Keyword and default arguments
=============================
It is possible to specify keyword and default arguments using the syntax
Expand Down
3 changes: 2 additions & 1 deletion include/pybind11/detail/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ inline void tls_replace_value(PYBIND11_TLS_KEY_REF key, void *value) {
// libstdc++, this doesn't happen: equality and the type_index hash are based on the type name,
// which works. If not under a known-good stl, provide our own name-based hash and equality
// functions that use the type name.
#if defined(__GLIBCXX__)
#if (PYBIND11_INTERNALS_VERSION <= 4 && defined(__GLIBCXX__)) \
|| (PYBIND11_INTERNALS_VERSION >= 5 && !defined(_LIBCPP_VERSION))
inline bool same_type(const std::type_info &lhs, const std::type_info &rhs) { return lhs == rhs; }
using type_hash = std::hash<std::type_index>;
using type_equal_to = std::equal_to<std::type_index>;
Expand Down
2 changes: 2 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ set(PYBIND11_TEST_FILES
test_tagbased_polymorphic
test_thread
test_union
test_unnamed_namespace_a
test_unnamed_namespace_b
test_vector_unique_ptr_member
test_virtual_functions)

Expand Down
2 changes: 1 addition & 1 deletion tests/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ def test_error_already_set_what_with_happy_exceptions(

@pytest.mark.skipif(
# Intentionally very specific:
"sys.version_info == (3, 12, 0, 'alpha', 6)",
"sys.version_info == (3, 12, 0, 'alpha', 7)",
reason="WIP: https://github.com/python/cpython/issues/102594",
)
@pytest.mark.skipif("env.PYPY", reason="PyErr_NormalizeException Segmentation fault")
Expand Down
38 changes: 38 additions & 0 deletions tests/test_unnamed_namespace_a.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#include "pybind11_tests.h"

namespace {
struct any_struct {};
} // namespace

TEST_SUBMODULE(unnamed_namespace_a, m) {
if (py::detail::get_type_info(typeid(any_struct)) == nullptr) {
py::class_<any_struct>(m, "unnamed_namespace_a_any_struct");
} else {
m.attr("unnamed_namespace_a_any_struct") = py::none();
}
m.attr("PYBIND11_INTERNALS_VERSION") = PYBIND11_INTERNALS_VERSION;
m.attr("defined_WIN32_or__WIN32") =
#if defined(WIN32) || defined(_WIN32)
true;
#else
false;
#endif
m.attr("defined___clang__") =
#if defined(__clang__)
true;
#else
false;
#endif
m.attr("defined__LIBCPP_VERSION") =
#if defined(_LIBCPP_VERSION)
true;
#else
false;
#endif
m.attr("defined___GLIBCXX__") =
#if defined(__GLIBCXX__)
true;
#else
false;
#endif
}
34 changes: 34 additions & 0 deletions tests/test_unnamed_namespace_a.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import pytest

from pybind11_tests import unnamed_namespace_a as m
from pybind11_tests import unnamed_namespace_b as mb

XFAIL_CONDITION = (
"(m.PYBIND11_INTERNALS_VERSION <= 4 and (m.defined___clang__ or not m.defined___GLIBCXX__))"
" or "
"(m.PYBIND11_INTERNALS_VERSION >= 5 and not m.defined_WIN32_or__WIN32"
" and "
"(m.defined___clang__ or m.defined__LIBCPP_VERSION))"
)
XFAIL_REASON = "Known issues: https://github.com/pybind/pybind11/pull/4319"


@pytest.mark.xfail(XFAIL_CONDITION, reason=XFAIL_REASON, strict=False)
@pytest.mark.parametrize(
"any_struct", [m.unnamed_namespace_a_any_struct, mb.unnamed_namespace_b_any_struct]
)
def test_have_class_any_struct(any_struct):
assert any_struct is not None


def test_have_at_least_one_class_any_struct():
assert (
m.unnamed_namespace_a_any_struct is not None
or mb.unnamed_namespace_b_any_struct is not None
)


@pytest.mark.xfail(XFAIL_CONDITION, reason=XFAIL_REASON, strict=True)
def test_have_both_class_any_struct():
assert m.unnamed_namespace_a_any_struct is not None
assert mb.unnamed_namespace_b_any_struct is not None
13 changes: 13 additions & 0 deletions tests/test_unnamed_namespace_b.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include "pybind11_tests.h"

namespace {
struct any_struct {};
} // namespace

TEST_SUBMODULE(unnamed_namespace_b, m) {
if (py::detail::get_type_info(typeid(any_struct)) == nullptr) {
py::class_<any_struct>(m, "unnamed_namespace_b_any_struct");
} else {
m.attr("unnamed_namespace_b_any_struct") = py::none();
}
}
5 changes: 5 additions & 0 deletions tests/test_unnamed_namespace_b.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from pybind11_tests import unnamed_namespace_b as m


def test_have_attr_any_struct():
assert hasattr(m, "unnamed_namespace_b_any_struct")

0 comments on commit 6de6191

Please sign in to comment.