Skip to content

Shuffling code in test_smart_ptr.cpp to separate struct/class definitions from bindings code. #2875

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.

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

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 24, 2021

  • Back-porting from smart_holder branch, to minimize diffs and the potential for merge conflicts.
  • Mostly code is just shuffled (see below for proof that nothing got lost).
  • All code outside TEST_MODULE moved to the anonymous namespace. This isn't strictly necessary for the smart_holder work, but best practice. All PYBIND11_DECLARE_HOLDER_TYPE were grouped together right after the anonymous namespace (to not have to close-reopen the namespace N times).
  • In 4 cases, std::unique_ptr is explicitly specified as holder (see diff below). On current master this is redundant, because std::unique_ptr is the default holder, but it enables testing against the smart_holder branch with -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT.
  • A few comments were duplicated for clarity, to appear both in the anonymous namespace and in TEST_MODULE. See diff below.

Changelog not needed.

Simple proof that nothing got lost:

cd pybind11/tests
git checkout master
cat test_smart_ptr.cpp | sed 's/^ *//' | grep -v '^$' | sort > zmaster
git checkout test_smart_ptr_non_interleaved
cat test_smart_ptr.cpp | sed 's/^ *//' | grep -v '^$' | sort > zpr
diff zmaster zpr
rm zmaster zpr

The full output from the diff command above:

37a38
> // #187: issue involving std::shared_ptr<> return value policy & garbage collection
54a56
> // but implement `struct`s and `class`es in the anonymous namespace above.
146a149
> // Issue #865: shared_from_this doesn't work with virtual inheritance
196a200,201
> } // namespace
> namespace {
207a213
> // Please do not interleave `struct` and `class` definitions with bindings code,
243c249
< py::class_<HeldByDefaultHolder>(m, "HeldByDefaultHolder")
---
> py::class_<HeldByDefaultHolder, std::unique_ptr<HeldByDefaultHolder>>(m, "HeldByDefaultHolder")
248c254
< py::class_<MyObject4b, MyObject4a>(m, "MyObject4b")
---
> py::class_<MyObject4b, MyObject4a, std::unique_ptr<MyObject4b>>(m, "MyObject4b")
252c258
< py::class_<SharedFromThisRef>(m, "SharedFromThisRef")
---
> py::class_<SharedFromThisRef, std::unique_ptr<SharedFromThisRef>>(m, "SharedFromThisRef")
254c260
< py::class_<SharedPtrRef>(m, "SharedPtrRef")
---
> py::class_<SharedPtrRef, std::unique_ptr<SharedPtrRef>>(m, "SharedPtrRef")
315a322,323
> // test_holder_with_addressof_operator
> // test_large_holder
317a326,327
> // test_move_only_holder
> // test_move_only_holder_with_addressof_operator
319a330
> // test_shared_ptr_and_references
320a332,333
> // test_shared_ptr_from_this_and_references
> // test_shared_ptr_gc
323a337
> // test_smart_ptr_from_default
327a342,343
> // test_unique_deleter
> // test_unique_nodelete
333a350
> // This helps keeping the smart_holder branch in sync with master.

…ions from bindings code. Back-porting from smart_holder branch, to minimize diffs and potential for merge conflicts.
@rwgk
Copy link
Collaborator Author

rwgk commented Feb 24, 2021

For easy reference, below is a snapshot of the diff of this PR against the smart_holder branch.

The PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS macro expands to nothing unless PYBIND11_USE_SMART_HOLDER_AS_DEFAULT is defined. Otherwise this is the expansion:

# define PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(T, ...) \

diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp
index e0af2497..e5e678ef 100644
--- a/tests/test_smart_ptr.cpp
+++ b/tests/test_smart_ptr.cpp
@@ -272,6 +272,38 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr<T>);
 PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator<T>);
 PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator<T>);
 
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(Object, ref<Object>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject1, ref<MyObject1>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject2, std::shared_ptr<MyObject2>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject3, std::shared_ptr<MyObject3>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject4, std::unique_ptr<MyObject4, py::nodelete>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject4a, std::unique_ptr<MyObject4a, py::nodelete>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject4b, std::unique_ptr<MyObject4b>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject5, huge_unique_ptr<MyObject5>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(SharedPtrRef::A, std::shared_ptr<SharedPtrRef::A>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(SharedPtrRef, std::unique_ptr<SharedPtrRef>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(SharedFromThisRef::B, std::shared_ptr<SharedFromThisRef::B>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(SharedFromThisRef, std::unique_ptr<SharedFromThisRef>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(SharedFromThisVirt, std::shared_ptr<SharedFromThisVirt>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(C, custom_unique_ptr<C>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(TypeForHolderWithAddressOf, shared_ptr_with_addressof_operator<TypeForHolderWithAddressOf>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(TypeForMoveOnlyHolderWithAddressOf, unique_ptr_with_addressof_operator<TypeForMoveOnlyHolderWithAddressOf>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(HeldByDefaultHolder, std::unique_ptr<HeldByDefaultHolder>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(ElementBase, std::shared_ptr<ElementBase>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(ElementA, std::shared_ptr<ElementA>)
+PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(ElementList, std::shared_ptr<ElementList>)
+
+#ifdef PYBIND11_USE_SMART_HOLDER_AS_DEFAULT
+// To prevent triggering a static_assert in the smart_holder code.
+// This is a very special case, because the associated test exercises a holder mismatch.
+namespace pybind11 { namespace detail {
+template <>
+class type_caster<std::shared_ptr<HeldByDefaultHolder>>
+    : public copyable_holder_caster<HeldByDefaultHolder, std::shared_ptr<HeldByDefaultHolder>> {};
+} // namespace detail
+} // namespace pybind11
+#endif
+
 TEST_SUBMODULE(smart_ptr, m) {
     // Please do not interleave `struct` and `class` definitions with bindings code,
     // but implement `struct`s and `class`es in the anonymous namespace above.

@wjakob
Copy link
Member

wjakob commented Feb 25, 2021

This looks pretty harmless, fine with me.

@rwgk rwgk merged commit e2e819b into pybind:master Feb 25, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 25, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Feb 25, 2021
@rwgk rwgk deleted the test_smart_ptr_non_interleaved branch February 25, 2021 15:16
rwgk added a commit that referenced this pull request Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants