Test: Default Alias in Base Class #4581
Conversation
ax3l
force-pushed
the
topic-template-alias-base
branch
4 times, most recently
from
cbfe3c7
to
92ead61
Compare
ax3l
force-pushed
the
topic-template-alias-base
branch
from
26c5b93
to
fd68d4c
Compare
This tests that template type aliases for base class templates are resolved to the same registered types.
ax3l
force-pushed
the
topic-template-alias-base
branch
from
c27f62d
to
e009a87
Compare
tests/test_template_alias_base.cpp
Outdated
| // this returns S<DefaultAllocator> even though we register | ||
| // the type S<std::allocator> | ||
| // MSVC and Clang on Windows failed here to detect the registered type | ||
| S<> make_S() { return S<>{}; } |
There was a problem hiding this comment.
This is basically stress-testing the compiler+library: can it figure out that S<DefaultAllocator> and S<std::allocator> are the same?
Question 1: Is it difficult to not have that stress-test baked into your downstream project? Either, by consistently avoiding the alias, or consistently using the alias.
Question 2: What happens if you patch internals.h similar to #4319?
If you decide to experiment more here, could you please not force push? That makes it more difficult for me to follow.
There was a problem hiding this comment.
This is basically stress-testing the compiler+library: can it figure out that S and Sstd::allocator are the same?
Correct.
Question 1: Is it difficult to not have that stress-test baked into your downstream project? Either, by consistently avoiding the alias, or consistently using the alias.
This is somewhat tricky in my case, especially across library/module interfaces. By consistently using one or the other, one has to hope a user does never use the other possible option of the alias.
Question 2: What happens if you patch internals.h similar to #4319?
Will try and push here.
If you decide to experiment more here, could you please not force push? That makes it more difficult for me to follow.
Will do! I just force-pushed initially yesterday when I realized the initial test draft was not yet complete/buggy.
There was a problem hiding this comment.
@rwgk I don't understand your patch yet:
The comment says, GNU's libstdc++ is good (#if defined(__GLIBCXX__)) - all other implementations need the self-made hash.
Your patch says LLVM's libc++ is not good (#if !defined(_LIBCPP_VERSION)) - but all other implementations can use the standard hashes.
There was a problem hiding this comment.
Yes: the CI for #4319 that ran yesterday (all successful) supports "LLVM's libc++ is not good (#if !defined(_LIBCPP_VERSION)) - but all other implementations can use the standard hashes."
But it looks like your added test doesn't work universally any way you tried, is that a correct summary?
There was a problem hiding this comment.
Nice!
My test works on Linux & Windows with libstdc++.
Co-authored-by: "Ralf W. Grosse-Kunstleve" <rwgk@google.com>
Below are the CI results 1. Failing, 2. Successful. It looks like a Clangs are failing. Which are Linux & libstdc++? CI / 🐍 3.6 • windows-2022 • x64 (pull_request) Failing after 6m CI / 🐍 3.6 • ubuntu-20.04 • x64 -DPYBIND11_FINDPYTHON=ON -DCMAKE_CXX_FLAGS="-D_=1" (pull_request) Successful in 11m |
|
Yes, the passing ones are with GCC and the Windows GCC port aka mingw32/64 ones. Definitely curious issue... |
Oh, sorry I got libstdc++ and libc++ mixed up in my mind between November and now. I just re-learned:
(I'm not sure this is strictly correct, but hopefully close enough.) |
|
What I find curious is the following: technically, all STLs should work if we always use our has implementation. But they do not, some fail. See last commit. I wonder if there is something on top of your |
Misunderstanding? PR #4319 doesn't have a fix for My test under PR #4319 exercises the behavior for types in the unnamed namespace. Unconditionally using the Your test is related but different. My current understanding: |
|
I think the issue with the test I have here is actually rooted somewhere in the function dispatch logic: |
Hm... My guess is that the |
|
Yep, this seems to be the location. |
|
A few observations: I checked out your branch and made the .cpp code like this: That passes the test. (No surprise.) But with I see a compiler error: Going back to diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 0b710d7e..941db900 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -928,6 +928,7 @@ public:
// polymorphic_type_hook). If the instance isn't derived, returns the base version.
static std::pair<const void *, const type_info *> src_and_type(const itype *src) {
const auto &cast_type = typeid(itype);
+ printf("\nLOOOK typeid.name()=%s clean=%s %s:%d\n", cast_type.name(), detail::clean_type_id(cast_type.name()).c_str(), __FILE__, __LINE__); fflush(stdout);
const std::type_info *instance_type = nullptr;
const void *vsrc = polymorphic_type_hook<itype>::get(src, instance_type);
if (instance_type && !same_type(cast_type, *instance_type)) {
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index fb8a75c0..c1f3fb44 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1364,6 +1364,7 @@ protected:
auto &internals = get_internals();
auto tindex = std::type_index(*rec.type);
+ printf("\nLOOOK rec.type->name()=%s clean=%s %s:%d\n", rec.type->name(), detail::clean_type_id(rec.type->name()).c_str(), __FILE__, __LINE__); fflush(stdout);
tinfo->direct_conversions = &internals.direct_conversions[tindex];
if (rec.module_local) {
get_local_internals().registered_types_cpp[tindex] = tinfo;I see: With this addditional diff diff --git a/tests/test_template_alias_base.cpp b/tests/test_template_alias_base.cpp
index f5d48f9d..475334fa 100644
--- a/tests/test_template_alias_base.cpp
+++ b/tests/test_template_alias_base.cpp
@@ -25,4 +25,7 @@ TEST_SUBMODULE(template_alias_base, m) {
py::class_<S<std::allocator>, Base<std::allocator>>(m, "S_std").def(py::init());
m.def("make_S", &make_S);
+
+ m.def("typeid_S_empty_eq_S_std_allocator", []() { return typeid(S<>) == typeid(S<std::allocator>); });
+ m.def("type_index_S_empty_eq_S_std_allocator", []() { return std::type_index(typeid(S<>)) == std::type_index(typeid(S<std::allocator>)); });
}
diff --git a/tests/test_template_alias_base.py b/tests/test_template_alias_base.py
index 2cae121d..284fbf52 100644
--- a/tests/test_template_alias_base.py
+++ b/tests/test_template_alias_base.py
@@ -9,3 +9,11 @@ def test_can_create_variable():
def test_can_return_variable():
v = m.make_S()
print(v)
+
+
+def test_typeid_eq():
+ assert m.typeid_S_empty_eq_S_std_allocator()
+
+
+def test_type_index_eq():
+ assert m.type_index_S_empty_eq_S_std_allocator()and clang (clang 14): But everything including those last two pass with gcc (gcc 12). |
|
I think this just boils down to the behaviors for: With gcc both are true. With clang and MSVC both are false. The main question: What does the C++ standard say about this, if anything? Related detail: is the clang compiler error compatible with the C++ standard? But for all practical purposes: I don't think there is an alternative to consistently using |
|
Actually, you could do something like: But that'll get ugly with the derived class |
Description
This tests that template type aliases for base class templates are resolved to the same registered types.
This shows problems for me on Windows with both MSVC and Clang (
clang-clin CMake).X-ref: #4319 (comment)
Suggested changelog entry: