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
| // 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 |
2 participants
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: