mirror of
https://github.com/pybind/pybind11.git
synced 2026-03-28 11:07:46 +00:00
master
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Ralf W. Grosse-Kunstleve
|
4dc33d6524 |
Fix smart_holder multiple/virtual inheritance bugs in shared_ptr and unique_ptr to-Python conversions (#5836)
* ChatGPT-generated diamond virtual-inheritance test case.
* Report "virtual base at offset 0" but don't skip test.
* Remove Left/Right virtual default dtors, to resolve clang-tidy errors:
```
/__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:44:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors]
44 | virtual ~Left() = default;
| ~~~~~~~ ^
| override
/__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:48:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors]
48 | virtual ~Right() = default;
| ~~~~~~~ ^
| override
```
* Add assert(ptr) in register_instance_impl, deregister_instance_impl
* Proper bug fix
* Also exercise smart_holder_from_unique_ptr
* [skip ci] ChatGPT-generated bug fix: smart_holder::from_unique_ptr()
* Exception-safe ownership transfer from unique_ptr to shared_ptr
ChatGPT:
* shared_ptr’s ctor can throw (control-block alloc). Using get() keeps unique_ptr owning the memory if that happens, so no leak.
* Only after the shared_ptr is successfully constructed do you release(), transferring ownership exactly once.
* [skip ci] Rename alias_ptr to mi_subobject_ptr to distinguish from trampoline code (which often uses the term "alias", too)
* [skip ci] Also exercise smart_holder::from_raw_ptr_take_ownership
* [skip ci] Add st.first comments (generated by ChatGPT)
* [skip ci] Copy and extend (raw_ptr, unique_ptr) reproducer from PR #5796
* Some polishing: comments, add back Left/Right dtors for consistency within test_class_sh_mi_thunks.cpp
* explicitly default copy/move for VBase to silence -Wdeprecated-copy-with-dtor
* Resolve clang-tidy error:
```
/__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:67:5: error: 'auto ptr' can be declared as 'auto *ptr' [readability-qualified-auto,-warnings-as-errors]
67 | auto ptr = new Diamond;
| ^~~~
| auto *
```
* Expand comment in `smart_holder::from_unique_ptr()`
* Better Left/Right padding to make it more likely that we avoid "all at offset 0". Clarify comment.
* Give up on `alignas(16)` to resolve MSVC warning:
```
"D:\a\pybind11\pybind11\build\ALL_BUILD.vcxproj" (default target) (1) ->
"D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj" (default target) (13) ->
(ClCompile target) ->
D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(70,17): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(80,43): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: 'std::_Ref_count_obj2<_Ty>': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: with [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: [ [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: _Ty=test_class_sh_mi_thunks::Diamond [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: ] [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
D:\a\pybind11\pybind11\include\pybind11\detail\init.h(77,21): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
```
The warning came from alignas(16) making Diamond over-aligned, while regular new/make_shared aren’t guaranteed to return 16-byte aligned memory on MSVC (hence C4316). I’ve removed the explicit alignment and switched to asymmetric payload sizes (char[4] vs char[24]), which still nudges MI layout without relying on over-alignment. This keeps the test goal and eliminates the warning across all MSVC builds. If we ever want to stress over-alignment explicitly, we can add aligned operator new/delete under __cpp_aligned_new, but that’s more than we need here.
* Rename test_virtual_base_at_offset_0() → test_virtual_base_not_at_offset_0() and replace pytest.skip() with assert. Add helpful comment for future maintainers.
|
||
|
Ralf W. Grosse-Kunstleve
|
2943a27a14 |
squash-merge smart_holder branch into master (#5542)
* Pure `git merge --squash smart_holder` (no manual interventions). * Remove ubench/ directory. * Remove include/pybind11/smart_holder.h * [ci skip] smart_ptrs.rst updates [WIP/unfinished] * [ci skip] smart_ptrs.rst updates continued; also updating classes.rst, advanced/classes.rst * Remove README_smart_holder.rst * Restore original README.rst from master * [ci skip] Minimal change to README.rst, to leave a hint that this is pybind11v3 * [ci skip] Work in ChatGPT suggestions. * Change macro name to PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE * Add a note pointing to the holder reinterpret_cast. * Incorporate suggestion by @virtuald: https://github.com/pybind/pybind11/pull/5542#discussion_r1967000989 * Systematically change most py::class_ to py::classh under docs/ * Remove references to README_smart_holder.rst This should have been part of commit |