Test type_caster bare interface #2834
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Jan 29, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Jan 29, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Jan 30, 2021
There was a problem hiding this comment.
Mostly some style nitpicking, in order to be consistent with the style in other pybind11 tests, and be more intuitively readable. Apart from that, this seems useful to have tested, yes, so thanks a lot for adding this long overdue test!
One content-wise thing: this seems to be a superset of type_caster<ConstRefCasted> in test_const_ref_caster, except for the reference_wrapper part. Should we remove that test, and just copy the return_reference_wrapper/pass_reference_wrapper function?
Bonus nitpick: pybind11 (reasonably) consistently uses west-side const, const Foo &bar style. I realize there's a whole discussion on which one is best and I won't take sides (though I happen to agree with pybind11), but mixing styles might be even worse?
| namespace pybind11_tests { | ||
| namespace type_caster_bare_interface { | ||
|
|
||
| struct mpty {}; |
There was a problem hiding this comment.
I have a hard time reading everything since 1) I have no idea how to pronounce/read this, 2) my brain's expecting capitals for "custom"/user-side classes in the test. Do you mind just turning (or I can also do it, if you don't have time) this into Foo or CustomClass or Value or so?
| std::unique_ptr<mpty> rtrn_uqmp() { return std::unique_ptr<mpty >(new mpty); } | ||
| std::unique_ptr<mpty const> rtrn_uqcp() { return std::unique_ptr<mpty const>(new mpty); } | ||
|
|
||
| const char* pass_uqmp(std::unique_ptr<mpty>) { return "load_uqmp"; } |
There was a problem hiding this comment.
Uuumpf, this takes me 2 second per function to parse to know what it's saying. Do you feel strongly about keeping 4 letters? Personally, I find pass_unique_ptr, pass_const_ref, pass_rvalue_ref, return_..., ... easier to directly read, even though things then don't nicely line out anymore.
But your effort on writing tests, so your final say.
Comment on lines
82
to
93
| using cast_op_type = conditional_t< | ||
| std::is_same<remove_reference_t<T_>, mpty const *>::value, | ||
| mpty const *, | ||
| conditional_t< | ||
| std::is_same<remove_reference_t<T_>, mpty *>::value, | ||
| mpty *, | ||
| conditional_t< | ||
| std::is_same<T_, mpty const &>::value, | ||
| mpty const &, | ||
| conditional_t<std::is_same<T_, mpty &>::value, | ||
| mpty &, | ||
| conditional_t<std::is_same<T_, mpty &&>::value, mpty &&, mpty>>>>>; |
There was a problem hiding this comment.
What about going else if style on this, to avoid indentation? (similar to test_builtin_casters.cpp)
Suggested change
| using cast_op_type = conditional_t< | |
| std::is_same<remove_reference_t<T_>, mpty const *>::value, | |
| mpty const *, | |
| conditional_t< | |
| std::is_same<remove_reference_t<T_>, mpty *>::value, | |
| mpty *, | |
| conditional_t< | |
| std::is_same<T_, mpty const &>::value, | |
| mpty const &, | |
| conditional_t<std::is_same<T_, mpty &>::value, | |
| mpty &, | |
| conditional_t<std::is_same<T_, mpty &&>::value, mpty &&, mpty>>>>>; | |
| using cast_op_type = | |
| conditional_t<std::is_same<remove_reference_t<T_>, mpty const *>::value, | |
| mpty const *, | |
| conditional_t<std::is_same<remove_reference_t<T_>, mpty *>::value, | |
| mpty *, | |
| conditional_t<std::is_same<T_, mpty const &>::value, | |
| mpty const &, | |
| conditional_t<std::is_same<T_, mpty &>::value, | |
| mpty &, | |
| conditional_t<std::is_same<T_, mpty &&>::value, | |
| mpty &&, | |
| /* else */ mpty>>>>>; |
| @@ -129,6 +129,7 @@ set(PYBIND11_TEST_FILES | |||
| test_stl.cpp | |||
| test_stl_binders.cpp | |||
| test_tagbased_polymorphic.cpp | |||
| test_type_caster_bare_interface.cpp | |||
There was a problem hiding this comment.
Final nitpick: test_type_caster_interface also works, if we want to have this file show off the interface?
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Jan 30, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Jan 30, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 1, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 2, 2021
|
Converting this to a Draft PR for now. While developing PR #2672 I find myself tweaking the systematic naming scheme. I will update this PR with matching names later. |
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 9, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 9, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 15, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 15, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 22, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 22, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 24, 2021
rwgk
pushed a commit
to rwgk/pybind11
that referenced
this pull request
Feb 24, 2021
rwgk
pushed a commit
that referenced
this pull request
Feb 24, 2021
* Adding test_unique_ptr_member (for desired PyCLIF behavior). See also: #2583 Does not build with upstream master or #2047, but builds with https://github.com/RobotLocomotion/pybind11 and almost runs: ``` Running tests in directory "/usr/local/google/home/rwgk/forked/EricCousineau-TRI/pybind11/tests": ================================================================================= test session starts ================================================================================= platform linux -- Python 3.8.5, pytest-5.4.3, py-1.9.0, pluggy-0.13.1 rootdir: /usr/local/google/home/rwgk/forked/EricCousineau-TRI/pybind11/tests, inifile: pytest.ini collected 2 items test_unique_ptr_member.py .F [100%] ====================================================================================== FAILURES ======================================================================================= _____________________________________________________________________________ test_pointee_and_ptr_owner ______________________________________________________________________________ def test_pointee_and_ptr_owner(): obj = m.pointee() assert obj.get_int() == 213 m.ptr_owner(obj) with pytest.raises(ValueError) as exc_info: > obj.get_int() E Failed: DID NOT RAISE <class 'ValueError'> test_unique_ptr_member.py:17: Failed ============================================================================= 1 failed, 1 passed in 0.06s ============================================================================= ``` * unique_ptr or shared_ptr return * new test_variant_unique_shared with vptr_holder prototype * moving prototype code to pybind11/vptr_holder.h, adding type_caster specialization to make the bindings involving unique_ptr passing compile, but load and cast implementations are missing * disabling GitHub Actions on pull_request (for this PR) * disabling AppVeyor (for this PR) * TRIGGER_SEGSEV macro, annotations for GET_STACK (vptr::get), GET_INT_STACK (pointee) * adding test_promotion_of_disowned_to_shared * Copying tests as-is from xxx_value_ptr_xxx_holder branch. https://github.com/rwgk/pybind11/tree/xxx_value_ptr_xxx_holder Systematically exercising returning and passing unique_ptr<T>, shared_ptr<T> with unique_ptr, shared_ptr holder. Observations: test_holder_unique_ptr: make_unique_pointee OK pass_unique_pointee BUILD_FAIL (as documented) make_shared_pointee Abort free(): double free detected pass_shared_pointee RuntimeError: Unable to load a custom holder type from a default-holder instance test_holder_shared_ptr: make_unique_pointee Segmentation fault (#1138) pass_unique_pointee BUILD_FAIL (as documented) make_shared_pointee OK pass_shared_pointee OK * Copying tests as-is from xxx_value_ptr_xxx_holder branch. https://github.com/rwgk/pybind11/tree/xxx_value_ptr_xxx_holder Systematically exercising casting between shared_ptr<base>, shared_ptr<derived>. * Demonstration of Undefined Behavior in handling of shared_ptr holder. Based on https://godbolt.org/z/4fdjaW by jorgbrown@ (thanks Jorg!). * Additional demonstration of Undefined Behavior in handling of shared_ptr holder. * fixing up-down mixup in comment * Demonstration of Undefined Behavior in handling of polymorphic pointers. (This demo does NOT involve smart pointers at all, unlike the otherwise similar test_smart_ptr_private_first_base.) * minor test_private_first_base.cpp simplification (after discovering that this can be wrapped with Boost.Python, using boost::noncopyable) * pybind11 equivalent of Boost.Python test similar to reproducer under #1333 * Snapshot of WIP, TODO: shared_ptr deleter with on/off switch * Adding vptr_deleter. * Adding from/as unique_ptr<T> and unique_ptr<T, D>. * Adding from_shared_ptr. Some polishing. * New tests/core/smart_holder_poc_test.cpp, using Catch2. * Adding in vptr_deleter_guard_flag. * Improved labeling of TEST_CASEs. * Shuffling existing TEST_CASEs into systematic matrix. * Implementing all [S]uccess tests. * Implementing all [E]xception tests. * Testing of exceptions not covered by the from-as matrix. * Adding top-level comment. * Converting from methods to factory functions (no functional change). * Removing obsolete and very incomplete test (replaced by Catch2-based test). * Removing stray file. * Adding type_caster_bare_interface_demo. * Adding shared_ptr<mpty>, shared_ptr<mpty const> casters. * Adding unique_ptr<mpty>, unique_ptr<mpty const> casters. * Pure copy of `class class_` implementation in pybind11.h (master commit 98f1bbb). * classh.h: renaming of class_ to classh + namespace; forking test_classh_wip from test_type_caster_bare_interface_demo. * Hard-coding smart_holder into classh. * Adding mpty::mtxt string member. * Adding isinstance<mpty> in type_caster::load functions. * Adding rvalue_ref, renaming const_value_ref to lvalue_ref & removing const. * Retrieving smart_holder pointer in type_caster<mpty>::load, and using it cast_op operators. * Factoring out smart_holder_type_caster_load. * Retrieving smart_holder pointer in type_caster<std::shared_ptr<mpty[ const]>>::load, and using it cast_op operators. * Improved error messaging: Cannot disown nullptr (as_unique_ptr). * Retrieving smart_holder pointer in type_caster<std::unique_ptr<mpty[ const]>>::load, and using it cast_op operators. * Pure `clang-format --style=file -i` change. * Pure `clang-format --style=file -i` change, with two `clang-format off` directives. * Fixing oversight (discovered by flake8). * flake8 cleanup * Systematically setting mtxt for all rtrn_mpty_* functions (preparation, the values are not actually used yet). * static cast handle for rtrn_cptr works by simply dropping in code from type_caster_base (marked with comments). * static cast handle for rtrn_cref works by simply dropping in code from type_caster_base (marked with comments). rtrn_mref and rtrn_mptr work via const_cast (to add const). * static cast handle for rtrn_valu works by simply dropping in code from type_caster_base (marked with comments). rtrn_rref raises a RuntimeError, to be investigated. * Copying type_caster_generic::cast into type_caster<mpty> as-is (preparation for handling smart pointers). * Pure clang-format change (applied to original type_caster_generic::cast). * Adding comment re potential use_count data race. * static handle cast implementations for rtrn_shmp, rtrn_shcp. * Adding MISSING comments in operator std::unique_ptr<mpty[ const]>. * static handle cast implementations for rtrn_uqmp, rtrn_uqcp. * Bug fix: vptr_deleter_armed_flag_ptr has to live on the heap. See new bullet point in comment section near the top. The variable was also renamed to reflect its function more accurately. * Fixing bugs discovered by ASAN. The code is now ASAN, MSAN, UBSAN clean. * Making test_type_caster_bare_interface_demo.cpp slightly more realistic, ASAN, MSAN, UBSAN clean. * Calling deregister_instance after disowning via unique_ptr. * Removing enable_shared_from_this stub, simplifying existing code, clang-format. Open question, with respect to the original code: https://github.com/pybind/pybind11/blob/76a160070b369f8d82b945c97924227e8b835c94/include/pybind11/pybind11.h#L1510 To me it looks like the exact situation marked as `std::shared_ptr<Good> gp1 = not_so_good.getptr();` here: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this The comment there is: `// undefined behavior (until C++17) and std::bad_weak_ptr thrown (since C++17)` Does the existing code have UB pre C++17? I'll leave handling of enable_shared_from_this for later, as the need arises. * Cosmetical change around helper functions. * Using type_caster_base<mpty>::src_and_type directly, removing copy. Also renaming one cast to cast_const_raw_ptr, for clarity. * Fixing clang-format oversight. * Using factored-out make_constructor (PR #2798), removing duplicate code. * Inserting additional assert to ensure a returned unique_ptr is always a new Python instance. * Adding minor comment (change to internals needed to distinguish uninitialized/disowned in error message). * Factoring out find_existing_python_instance(). * Moving factored-out make_constructor to test_classh_wip.cpp, restoring previous version of cast.h. This is currently the most practical approach. See PR #2798 for background. * Copying classh type_casters from test_classh_wip.cpp UNMODIFIED, as a baseline for generalizing the code. * Using pybind11/detail/classh_type_casters.h from test_classh_wip.cpp. * Adding & using PYBIND11_CLASSH_TYPE_CASTERS define. * Adding test_classh_inheritance, currently failing (passes with class_). * Removing .clang-format before git rebase master (where the file was added). * Bringing back .clang-format, the previous rm was a bad idea. * Folding in modified_type_caster_generic_load_impl, just enough to pass test_class_wip. test_classh_inheritance is still failing, but with a different error: [RuntimeError: Incompatible type (as_raw_ptr_unowned).] * Minimal changes needed to pass test_classh_inheritance. * First pass adjusting try_implicit_casts and try_load_foreign_module_local to capture loaded_v_h, but untested and guarded with pybind11_failure("Untested"). This was done mainly to determine general feasibility. Note the TODO in pybind11.h, where type_caster_generic::local_load is currently hard-coded. test_classh_wip and test_classh_inheritance still pass, as before. * Decoupling generic_type from type_caster_generic. * Changes and tests covering classh_type_casters try_implicit_casts. * Minimal test covering classh_type_casters load_impl Case 2b. * Removing stray isinstance<T>(src): it interferes with the py::module_local feature. Adding missing #includes. * Tests for classh py::module_local() feature. * Pure renaming of function names in test_classh_inheritance, similar to the systematic approach used in test_class_wip. NO functional changes. * Pure renaming of function and variable names, for better generalization when convoluting with inheritance. NO functional changes. * Adopting systematic naming scheme from test_classh_wip. NO functional changes. * Moving const after type name, for functions that cover a systematic scheme. NO functional changes. * Adding smart_holder_type_caster_load::loaded_as_shared_ptr, currently bypassing smart_holder shared_ptr tracking completely, but the tests pass and are sanitizer clean. * Removing rtti_held from smart_holder. See updated comment. * Cleaning up loaded_as_raw_ptr_unowned, loaded_as_shared_ptr. * Factoring out convert_type and folding into loaded_as_unique_ptr. * Folding convert_type into lvalue_ref and rvalue_ref paths. Some smart_holder_type_caster_load cleanup. * Using unique_ptr in local_load to replace static variable. Also adding local_load_safety_guard. * Converting test_unique_ptr_member to using classh: fully working, ASAN, MSAN, UBSAN clean. * Removing debugging comments (GET_STACK, GET_INT_STACK). cast.h is identical to current master again, pybind11.h only has the generic_type::initialize(..., &type_caster_generic::local_load) change. * Purging obsolete pybind11/vptr_holder.h and associated test. * Moving several tests to github.com/rwgk/rwgk_tbx/tree/main/pybind11_tests rwgk/rwgk_tbx@a2c2f88 These tests are from experimenting, and for demonstrating UB in pybind11 multiple inheritance handling ("first_base"), to be fixed later. * Adding py::smart_holder support to py::class_, purging py::classh completely. * Renaming files in include directory, creating pybind11/smart_holder.h. * Renaming all "classh" to "smart_holder" in pybind11/detail/smart_holder_type_casters.h. The user-facing macro is now PYBIND11_SMART_HOLDER_TYPE_CASTERS. * Systematically renaming tests to use "class_sh" in the name. * Renaming test_type_caster_bare_interface_demo to test_type_caster_bare_interface. * Renaming new tests/core subdirectory to tests/pure_cpp. * Adding new tests to CMake config, resetting CI config. * Changing CMake file so that test_class_sh_module_local.py actually runs. * clang-tidy fixes. * 32-bit compatibility. * Reusing type_caster_base make_copy_constructor, make_move_constructor with a trick. * CMake COMPARE NATURAL is not available with older versions. * Adding copyright notices to new header files. * Explicitly define copy/move constructors/assignments. * Adding new header files to tests/extra_python_package/test_files.py. * Adding tests/pure_cpp/CMakeLists.txt. * Making use of the new find_existing_python_instance() function factored out with PR #2822. * Moving define PYBIND11_SMART_HOLDER_TYPE_CASTERS(T) down in the file. NO functional changes. Preparation for follow-up work (to keep that diff smaller). * Reintroducing py::classh, this time as a simple alias for py::class_<U, py::smart_holder>. * Replacing detail::is_smart_holder<H> in cast.h with detail::is_smart_holder_type_caster<T>. Moving get_local_load_function_ptr, init_instance_for_type to smart_holder_type_caster_class_hooks. Expanding static_assert in py::type::handle_of<> to accommodate smart_holder_type_casters. * Fixing oversight. * Adding classu alias for class_<U, std::unique_ptr<U>>. * Giving up on idea to use legacy init_instance only if is_base_of<type_caster_generic, type_caster<T>. There are use cases in the wild that define both a custom type_caster and class_. * Removing test_type_caster_bare_interface, which was moved to the separate PR #2834. * Moving up is_smart_holder_type_caster, to also use in cast_is_temporary_value_reference. * Adding smart_holder_type_casters for unique_ptr with custom deleter. SEVERE CODE DUPLICATION. This commit is to establish a baseline for consolidating the unique_ptr code. * Unification of unique_ptr, unique_ptr_with_deleter code in smart_holder_poc.h. Leads to more fitting error messages. Enables use of unique_ptr<T, D> smart_holder_type_casters also for unique_ptr<T>. * Copying files as-is from branch test_unique_ptr_member (PR #2672). * Adding comment, simplifying naming, cmake addition. * Introducing PYBIND11_USE_SMART_HOLDER_AS_DEFAULT macro (tested only undefined; there are many errors with the macro defined). * Removing test_type_caster_bare_interface, which was moved to the separate PR #2834. * Fixing oversight introduced with commit 95425f1. * Setting record.default_holder correctly for PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. With this test_class.cpp builds and even mostly runs, except `test_multiple_instances_with_same_pointer`, which segfaults because it is using a `unique_ptr` holder but `smart_holder` `type_caster`. Also adding `static_assert`s to generate build errors for such situations, but guarding with `#if 0` to first pivot to test_factory_constructors.cpp. * Fixing up cast.h and smart_holder.h after rebase. * Removing detail/smart_holder_type_casters.h in separate commit. * Commenting out const in def_buffer(... const). With this, test_buffers builds and runs with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. Explanation why the const needs to be removed, or fix elsewhere, is still needed, but left for later. * Adding test_class_sh_factory_constructors, reproducing test_factory_constructors failure. Using py::class_ in this commit, to be changed to py::classh for debugging. * Removing include/pybind11/detail/smart_holder_type_casters.h from CMakeLists.txt, test_files.py (since it does not exist in this branch). * Adding // DANGER ZONE reminders. * Converting as many py::class_ to py::classh as possible, not breaking tests. * Adding initimpl::construct() overloads, resulting in test_class_sh_factory_constructors feature parity for py::class_ and py::classh. * Adding enable_if !is_smart_holder_type_caster to existing initimpl::construct(). With this test_factory_constructors.cpp builds with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. * Disabling shared_ptr&, shared_ptr* tests when building with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT for now, pending work on smart_holder_type_caster<shared_ptr>. * Factoring out struct and class definitions into anonymous namespace. Preparation for building with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. * Simplifying from_unique_ptr(): typename D = std::default_delete<T> is not needed. Factoring out is_std_default_delete<T>() for consistentcy between ensure_compatible_rtti_uqp_del() and from_unique_ptr(). * Introducing PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS. Using it in test_smart_ptr.cpp. With this test_smart_ptr builds with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT and all but one test run successfully. * Introducing 1. type_caster_for_class_, used in PYBIND11_MAKE_OPAQUE, and 2. default_holder_type, used in stl_bind.h. * Using __VA_ARGS__ in PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS. * Replacing condense_for_macro with much simpler approach. * Softening static_assert, to only check specifically that smart_holder is not mixed with type_caster_base, and unique_ptr/shared_ptr holders are not mixed with smart_holder_type_casters. * Adding PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS in test_class.cpp (with this all but one test succeed with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT). * Adding remaining PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS. static_assert for "necessary conditions" for both types of default holder, static_assert for "strict conditions" guarded by new PYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX. All tests build & run as before with unique_ptr as the default holder, all tests build for smart_holder as the default holder, even with the strict static_assert. * Introducing check_is_smart_holder_type_caster() function for runtime check, and reinterpreting record.default_holder as "uses_unique_ptr_holder". With this test_smart_ptr succeeds. (All 42 tests build, 35 tests succeed, 5 run but have some failures, 2 segfault.) * Bug fix: Adding have_value() to smart_holder_type_caster_load. With this test_builtin_casters succeeds. (All 42 tests build, 36 tests succeed, 5 run but have some failures, 1 segfault.) * Adding unowned_void_ptr_from_direct_conversion to modified_type_caster_generic_load_impl. This fixes the last remaining segfault (test_numpy_dtypes). New stats for all tests combined: 12 failed, 458 passed. * Adding "Lazy allocation for unallocated values" (for old-style __init__) into load_value_and_holder. Deferring destruction of disowned holder until clear_instance, to remain inspectable for "uninitialized" or "disowned" detection. New stats for all tests combined: 5 failed, 465 passed. * Changing std::shared_ptr pointer/reference to const pointer/reference. New stats for all tests combined: 4 failed, 466 passed. * Adding return_value_policy::move to permissible policies for unique_ptr returns. New stats for all tests combined: 3 failed, 467 passed. * Overlooked flake8 fixes. * Manipulating failing ConstructorStats test to pass, to be able to run all tests with ASAN. This version of the code is ASAN clean with unique_ptr or smart_holder as the default. This change needs to be reverted after adopting the existing move-only-if-refcount-is-1 logic used by type_caster_base. * Adding copy constructor and move constructor tracking to atyp. Preparation for a follow-up change in smart_holder_type_caster, to make this test sensitive to the changing behavior. [skip ci] * Removing `operator T&&() &&` from smart_holder_type_caster, for compatibility with the behavior of type_caster_base. Enables reverting 2 of 3 test manipulations applied under commit 249df7c. The manipulation in test_factory_constructors.py is NOT reverted in this commit. [skip ci] * Fixing unfortunate editing mishap. This reverts the last remaining test manipulation in commit 249df7c and makes all existing unit tests pass with smart_holder as default holder. * GitHub CI clang-tidy fixes. * Adding messages to terse `static_assert`s, for pre-C++17 compatibility. * Using @pytest.mark.parametrize to run each assert separately (to see all errors, not just the first). * Systematically removing _atyp from function names, to make the test code simpler. * Using re.match to accommodate variable number of intermediate MvCtor. * Also removing `operator T()` from smart_holder_type_caster, to fix gcc compilation errors. The only loss is pass_rref in test_class_sh_basic. * Systematically replacing `detail::enable_if_t<...smart_holder...>` with `typename std::enable_if<...smart_holder...>::type`. Attempt to work around MSVC 2015 issues, to be tested via GitHub CI. The idea for this change originates from this comment: #1616 (comment) * Importing re before pytest after observing a PyPy CI flake when importing pytest first. * Copying MSVC 2015 compatibility change from branch pr2672_use_smart_holder_as_default. * Introducing is_smart_holder_type_caster_base_tag, to keep smart_holder code more disconnected. * Working around MSVC 2015 bug. * Expanding comment for MSVC 2015 workaround. * Systematically changing std::enable_if back to detail::enable_if_t, effectively reverting commit 5d4b689. * Removing unused smart_holder_type_caster_load::loaded_as_rvalue_ref (it was an oversight that it was not removed with commit 23036a4). * Removing py::classu, because it does not seem useful enough. * Reverting commit 6349531 by un-commenting `const` in `def_buffer(...)`. To make this possible, `operator T const&` and `operator T const*` in `smart_holder_type_caster` need to be marked as `const` member functions. * Adding construct() overloads for constructing smart_holder from alias unique_ptr, shared_ptr returns. * Adding test_class_sh_factory_constructors.cpp to tests/CMakeLists.txt (fixes oversight, this should have been added long before). * Compatibility with old clang versions (clang 3.6, 3.7 C++11). * Cleaning up changes to existing unit tests. * Systematically adding SMART_HOLDER_WIP tag. Removing minor UNTESTED tags (only the throw are not actually exercised, investing time there has a high cost but very little benefit). * Splitting out smart_holder_type_casters again, into new detail/smart_holder_type_casters_inline_include.h. * Splitting out smart_holder_init_inline_include.h. * Adding additional new include files to CMakeLists.txt, tests/extra_python_package/test_files.py. * clang-format cleanup of most smart_holder code. * Adding source code comments in response to review. * Simple micro-benchmark ("ubench") comparing runtime performance for several holders. Tested using github.com/rwgk/pybind11_scons and Google-internal build system. Sorry, no cmake support at the moment. First results: https://docs.google.com/spreadsheets/d/1InapCYws2Gt-stmFf_Bwl33eOMo3aLE_gc9adveY7RU/edit#gid=0 * Breaking out number_bucket.h, adding hook for also collecting performance data for PyCLIF. * Accounting for ubench in MANIFEST.in (simply prune, for now). * Smarter determination of call_repetitions. [skip ci] * Also scaling performance data to PyCLIF. [skip ci] * Adding ubench/python/number_bucket.clif here for general visibility. * Fix after rebase * Merging detail/smart_holder_init_inline_include.h into detail/init.h. * Renaming detail/is_smart_holder_type_caster.h -> detail/smart_holder_sfinae_hooks_only.h. * Renaming is_smart_holder_type_caster -> type_uses_smart_holder_type_caster for clarity. * Renaming type_caster_type_is_smart_holder_type_caster -> wrapped_type_uses_smart_holder_type_caster for clarity. * Renaming is_smart_holder_type_caster_base_tag -> smart_holder_type_caster_base_tag for simplicity. * Adding copyright notices and minor colateral cleanup. * iwyu cleanup (comprehensive only for cast.h and smart_holder*.h files). * Fixing `git rebase master` accident. * Moving large `pragma warning` block from pybind11.h to detail/common.h. * Fixing another `git rebase master` accident.
rhaschke
reviewed
Mar 8, 2021
There was a problem hiding this comment.
I think, testing that rtrn_*() calls the correct casting operation is fine.
However, changing the function signature of pass_*() will still call the "old" functions.
I don't see a value in testing this. Rather, it should be tested whether the values are correctly copied/moved when passing from python to C++.
Comment on lines
16
to
21
| assert m.pass_valu(None) == "load_valu" | ||
| assert m.pass_rref(None) == "load_rref" | ||
| assert m.pass_cref(None) == "load_cref" | ||
| assert m.pass_mref(None) == "load_mref" | ||
| assert m.pass_cptr(None) == "load_cptr" | ||
| assert m.pass_mptr(None) == "load_mptr" |
There was a problem hiding this comment.
What is actually tested here? This is just about calling different functions and comparing their return values, isn't it?
Comment on lines
30
to
31
| assert m.pass_shmp(None) == "load_shmp" | ||
| assert m.pass_shcp(None) == "load_shcp" |
Comment on lines
40
to
41
| assert m.pass_uqmp(None) == "load_uqmp" | ||
| assert m.pass_uqcp(None) == "load_uqcp" |
Comment on lines
88
to
93
| conditional_t< | ||
| std::is_same<T_, mpty const &>::value, | ||
| mpty const &, | ||
| conditional_t<std::is_same<T_, mpty &>::value, | ||
| mpty &, | ||
| conditional_t<std::is_same<T_, mpty &&>::value, mpty &&, mpty>>>>>; |
There was a problem hiding this comment.
Isn't all this just returning the same type as detected?
There was a problem hiding this comment.
Yes, that's what I'm thinking, too. I got this from PR #2705. It seemed to me exactly what I want, but I'm definitely still learning.
40f59aa
to
5d21d8a
Compare
|
Hi Robert, I reworked this PR quite a bit, to be less the stepping stone it was for me back in December, and a more meaningful test / demo (and be more similar to what's now test_class_sh_basic on the smart_holder branch). Could you please take another look? I'm now tracking The valgrind CI failure seems unrelated, btw; something todo with pypocketfft. |
rwgk
pushed a commit
to rwgk/rwgk_tbx
that referenced
this pull request
Jul 2, 2021
The PR will be closed. Using this repo as attic. Original git log: ``` commit 7fe7c96ad47f1e289f23a779566ec9ad3207a0af Author: Ralf W. Grosse-Kunstleve <[email protected]> Date: Mon Mar 8 11:38:40 2021 -0800 Tracing CpCtor, MvCtor, removing smart-pointer casters, renaming, more comments. commit 64abe68d7236725825a545f15891e8ee683e45a9 Author: Ralf W. Grosse-Kunstleve <[email protected]> Date: Fri Jan 29 14:00:26 2021 -0800 Adding comment, simplifying naming, cmake addition. commit 7c1471ea612ed9a72c6559ba515c86ba6255f28f Author: Ralf W. Grosse-Kunstleve <[email protected]> Date: Fri Jan 29 13:42:52 2021 -0800 Copying files as-is from branch test_unique_ptr_member (PR #2672). ```
|
Closing this stale PR to reduce distractions. Files archived here as-is: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Systematically exercises the
detail::type_caster<> interface(which is a de-facto public interface). This is going a step in the direction of an integration test, to ensure multiple components of pybind11 work together correctly. It is also useful to show thetype_caster<>interface virtually clutter-free.This new test was copied from PR #2672, with small modifications.