Adjusting `type_caster<std::reference_wrapper<T>>` to support const/non-const propagation in `cast_op`. #2705

laramiel · 2020-11-30T18:48:42Z

Description

Before std::reference_wrapper<T> would invoke cast_op<type> rather than cast_op<T&> to construct the conversion operator type. This introduces different behavior for reference wrapped types when the underlying type_caster<>::cast_op<T>& differs from type_caster<>::cast_op<T&>.

There is additional enforcement from a static_assert that requires the type resulting from type_caster<>::cast_op<T> to be equivalent to intrinsic_t<T> (essentially std:decay<T>).

After, the type_caster<> specialization invokes cast_op<type&>, which allows std::reference_wrapper<> to behave in the same way as a native reference type.

Also, the static_assert is changed to allow the resulting type to be either T& or const T&, which gives slightly more flexibility to type_caster overloads.

This change is expected to be low impact for existing code and tests. Existing tests use a use the default implementation of cast_op which applies intrinsic_t to the result, effectively ignoring const, and making

The added test case, test_freezable_type_caster, demonstrates custom casting to either mutable or immutable types, and is a cleanroom test based somewhat on ideas from https://github.com/protocolbuffers/protobuf/tree/master/python/google/protobuf/pyext

…tive reference. Before, both std::reference_wrapper<T> and std::reference_wrapper<const T> would invoke cast_op<type>. This doesn't allow the type_caster<> specialization for T to distinguish reference_wrapper types from value types. After, the type_caster<> specialization invokes cast_op<type&>, which allows reference_wrapper to behave in the same way as a native reference type.

rwgk

Hi Laramie, I tried out your change locally, with small changes applied on top:

rwgk@86299e3

That commit adds back the original code with #if #else #endif, and uncomments one of your additional tests.

I found that your additional tests work with and without your change to type_caster<std::reference_wrapper<type>>. Is that expected? Would it be possible to add tests that fail without your change?

For completeness: I was using clang version 9.0.1 (gLinux rodette) with -std=c++17.

laramiel · 2020-12-01T04:48:28Z

... except you changed the test. The behavior below fails:

assert m.refwrap_lvalue_const().value == 1
m.refwrap_lvalue_const().value = 2  # Should fail with a ValueError()
assert m.refwrap_lvalue_const().value == 1   # not 2

The problem with your uncommented test is that pybind11 doesn't really have a mechanism to note mutable vs. immutable value types that correspond to c++ references vs c++ const references. It's possible to construct them by implementing a custom type_caster that manages a lot of the complexity itself. A skeleton might look like this:

template <>
struct type_caster<UserType> {

  static constexpr auto name = _<UserType>();

  // C++ -> Python
  static handle cast(const UserType *src, return_value_policy policy,
                     handle parent) {
     // ... construct an immutable python type referencing src.  This isn't really available in pybind11
    // and would have to be built by hand.
  }
  static handle cast(const UserType &src, return_value_policy policy, handle parent) {
   return cast(&src, policy, parent);
  }

  static handle cast(UserType *src, return_value_policy policy,
                     handle parent) {
    // ... construct a mutable python type referencing src. This is the default pybind11 path.
  }
  static handle cast(UserType &src, return_value_policy policy, handle) {
   return cast(&src, policy, parent);
  }

  static handle cast(UserType &&src, return_value_policy policy, handle) {
    // construct a mutable python type owning src.
  }

  // Convert Python->C++.
  bool load(handle src, bool convert) {
  	// ... either reference a wrapped c++ type or construct a new one. Good luck. :)
  }

  // cast_op_type determines which operator overload to call for a given c++
  // input parameter type. In this case we want to propagate const, etc.
  template <typename T_>
  using cast_op_type =
      std::conditional_t<
          std::is_same_v<std::remove_reference_t<T_>, const UserType *>,
              const UserType *,
      std::conditional_t<
          std::is_same_v<std::remove_reference_t<T_>, UserType *>, UserType *,
      std::conditional_t<
          std::is_same_v<T_, const UserType &>, const UserType &,
      std::conditional_t<std::is_same_v<T_, UserType &>, UserType &,
      /*default is T&&*/ T_>>>>;


  // PYBIND11_TYPE_CASTER
  operator const UserType *() { return value; }
  operator const UserType &() {
    if (!value) throw reference_cast_error();
    return *value;
  }
  operator ProtoType *() { return value; }
  operator ProtoType &() {
    if (!value) throw reference_cast_error();
    return *const_cast<UserType*>(value);
  }
  operator UserType &&() && {
    if (!value) throw reference_cast_error();
    owned = *value;
    return std::move(owned);
  }

 protected:
  const UserType *value;
  UserType owned;
};

But, well, putting that into the test is a bit much.

Anyway, yes, the tests (other than the uncommented one) are expected to pass in both cases.

rwgk · 2020-12-01T19:14:58Z

My take:

The commented test is best uncommented and changed so that it passes, even if we don't like the current behavior. In a terse comment we can acknowledge and explain. That way people know exactly what to expect. — Note that the loss of const isn't just limited to this situation. It existed in Boost.Python already, and is very difficult to avoid. (I wouldn't try.)
Adding a test for the situation you're making the change for is important, even if the test involves many more lines of code than the behavior change itself. The test is the only protection you'll have against someone later breaking the new behavior you want to rely on.

I'll add two other maintainers as reviewers, maybe they have different takes. Two approvals from maintainers are needed to merge.

This test is a chimera; it blends the pybind11 casters with a custom pytype implementation that supports immutable and mutable calls. In order to detect the immutable/mutable state, the cast_op needs to propagate it, even through e.g. std::reference<const T> Note: This is still a work in progress; some things are crashing, which likely means that I have a refcounting bug or something else missing.

Fixes the bugs in my custom python type implementation, demonstrate test that requires const& and reference_wrapper<const T> being treated differently from Non-const.

laramiel · 2020-12-02T04:48:06Z

Ok, this test_chimera demonstrates the requirement by creating a custom type and using pybind11 type_caster to convert between it and the c++ type. The custom type holds a pointer to an c++ object and a flag that indicates mutability.

Apply formatting presubmit check.

laramiel · 2020-12-02T04:53:56Z

... And I've pushed the other change that demonstrates that const-references are made non-const by pybind11.

rwgk

The suggestions are purely for the tests and mostly even just about naming and wording of comments. The core change is definitely fine. I ran it through the Google-global testing and found no issues.

rwgk · 2020-12-03T15:59:07Z

include/pybind11/cast.h

+                             subcaster_cast_op_type>::value ||
+                std::is_same<reference_t, subcaster_cast_op_type>::value),
+               "std::reference_wrapper<T> caster requires T to have a caster "
+               "with an `T &` operator or `const T&` operator");


"with operator `T &` or `const T&`");

rwgk · 2020-12-03T16:01:58Z

tests/CMakeLists.txt

@@ -96,6 +96,7 @@ set(PYBIND11_TEST_FILES
    test_constants_and_functions.cpp
    test_copy_move.cpp
    test_custom_type_casters.cpp
+    test_chimera.cpp


This name stands out as the only one that is non-descriptive of what is being tested. The name is fitting if the context of the test is known first, but doesn't help people understand what the test is doing without looking inside. Could you please find a descriptive name, in line with the existing test names?

rwgk · 2020-12-03T16:39:48Z

tests/test_builtin_casters.py

+    m.refwrap_lvalue().value = 4
+    assert m.refwrap_lvalue().value == 4
+
+    # const-ness is not propagated.


At second thought, I think it's best to only exercise

assert m.refwrap_lvalue_const().value == 1

and to delete the comment and the other two lines.
Rationale: You want to exercise that the caster changed in this PR is fully covered (load and cast for T& and const T&). The more general issue/feature that const is lost isn't relevant to this purpose.

rwgk · 2020-12-03T17:09:22Z

tests/test_chimera.cpp

@@ -0,0 +1,408 @@
+/*
+    tests/test_chimera.cpp -- This demonstrates a hybrid usage of pybind11, where the


Casters for types not wrapped with py::class_ are perfectly normal: chrono.h, complex.h, eigen.h, functional.h, numpy.h, stl.h and probably many more in client code.

I think it's nice to have this test, even though it's more an end-to-end test than a unit test, but it's important to explain here, for example starting with:

This original purpose of this test is to exercise that type_caster<std::reference_wrapper<T>> works for sub-casters that only have a const T& operator, but not a T& operator. ... continue with explanation why this is not a more focused unit test ...

rwgk · 2020-12-03T17:30:20Z

tests/test_chimera.cpp

+#pragma GCC diagnostic pop
+#endif
+
+static std::unordered_map<Chimera*, void*>* mapping =


The shorter
static std::unordered_map<Chimera*, void*> mapping;
(with mapping-> below replaced by mapping) also works. Is there a strong reason for allocating mapping on the heap? (Its size is only 56 bytes in a 64-bit build.)

Similarly for the shared and shared_const variables below.

rwgk · 2020-12-03T17:32:57Z

tests/test_chimera.cpp

+#include "pybind11_tests.h"
+
+/// C++ type
+class Chimera {


class FreezableInt
?
"Chimara" fits for the situation as a whole, but not for this type by itself.
Meaningful naming has a huge impact on readability, especially if this is meant to be a demo.

rwgk

@henryiii or @bstaletic, is there a chance you could help giving this a second look? The core change (in cast.h) is very small. I double-checked that the new test fails without the core change, i.e. we certainly have full coverage for the change.

rwgk · 2020-12-03T20:03:38Z

tests/test_freezable_type_caster.py

+
+    # by reference, so it's not converted.
+    with pytest.raises(TypeError):
+        assert m.roundtrip_ref(m.get_const_ref()).x == 4


The assert isn't actually needed, this by itself raises the TypeError:

m.roundtrip_ref(m.get_const_ref())

tests/test_freezable_type_caster.cpp

rwgk · 2020-12-04T08:01:47Z

I saw the CI / 🐍 pypy2 • ubuntu-latest • x64 failure we're getting here also on current pure master. It is definitely unrelated to this PR.

This test verifies that cast_op may be used to correctly detect const reference types when used with std::reference_wrapper.

bstaletic · 2020-12-14T20:40:41Z

tests/test_const_ref_caster.cpp

@@ -0,0 +1,88 @@
+/*
+    tests/test_const_ref_caster.cpp


These tests seem more appropriate in test_builtin_casters.cpp/test_builtin_casters.py.

Perhaps, but I think that since they are intended to exercise different aspects, putting it into a separate file makes the intent a bit more clear.

Yes, pybind11's tests are a mess. But I think the idea it to keep a minimal amount of files, because they all take quite some time to compile. I also don't think this one test is enough to create a separate file for.

Meanwhile, type_caster<std::reference_wrapper<T>> is already tested in test_builtin_casters.cpp and other test files already have custom casters as well (test_custom_type_casters.cpp, test_copy_move.cpp, and even one in pybind11_tests.h). So if we can keep the scope of the caster & test focused enough, I think it's fine.

The other counter point is that this requires a custom type caster; so maybe move them to custom_type_casters.cc? I think that the current test is about as minimal as it can be while exercising the various paths we want to test.

Just logging my view, not a request: it makes me sad to see this test lumped in with other things. I know the existing tests do that a lot, but I think it really isn't a good approach, because it convolutes unrelated things, makes them harder to understand/follow, and obviously is counter the the idea of testing small self-contained units. Sticking with the established pattern only has so much value. Nudging things in a better direction has value, too. My preference is definitely to keep this test separate, if Boris or Yannick can agree.

Anyway, Done.

I agree with @rwgk however I will change it to the reviewers consensus.

The other counter point is that this requires a custom type caster; so maybe move them to custom_type_casters.cc? I think that the current test is about as minimal as it can be while exercising the various paths we want to test.

Well, coming from this PR, it's testing the built-in caster for std::reference_wrapper, so I thought it would be best to keep this as close to the other tests for reference_wrapper, and to not create yet another file is "something with casters" (which takes time to compile, each time!).

move them to custom_type_casters.cc

It's not really about the custom caster, though. It's about testing reference_wrapper, currently, I though? If this maybe gets expanded further in the future, the caster could be shared in a header, though, when it's necessary.

it makes me sad to see this test lumped in with other things.

Why exactly? Because I had explicitly tried to design/shrink this to be as minimal, unit-testy as possible, such that it could fit in with the rest of the std::reference_wrapper tests (that were even extended) in builtin_casters.cpp.

And to me it's more confusing to break convention than to add a few lines to the existing tests.

I do agree the tests are messy, btw, but I don't see the urgency of splitting things up. Build times are (partially) to blame, I think. So maybe once #2445 gets merged, it will be more feasible to split up tests, in a separate PR that brings order to the tests in a less ad-hoc fashion?

I agree with @rwgk however I will change it to the reviewers consensus.

Not sure there's consensus, though. If you want, we can give @henryiii a decisive vote, but ... well, yeah, I've explained my reasoning :-)

tests/test_builtin_casters.cpp

1. std::add_lvalue_reference<type> -> type& 2. Simplify the test a little more; we're never returning the ConstRefCaster type so the class_ definition can be removed.

rwgk · 2020-12-15T01:14:23Z

The new test is an awesome demo for how cast_op works! It's really helping me understanding the caster system better. Another argument for not burring it in a bigger thing, because it'll be harder to work/experiment with.

For myself I changed it around like this:

struct ConstRefCasted {                                                         
  int unique_tag;                                                               
};

  operator ConstRefCasted&&() { value = {1}; return std::move(value); }         
  operator ConstRefCasted&() { value = {2}; return value; }                     
  operator ConstRefCasted*() { value = {3}; return &value; }                    
                                                                                
  operator const ConstRefCasted&() { value = {4}; return value; }               
  operator const ConstRefCasted*() { value = {5}; return &value; }

  m.def("takes", [](ConstRefCasted x) {                                         
    return x.unique_tag;                                                        
  });
  ...etc...

    utw = m.ConstRefCasted()                                                    
                                                                                
    assert m.takes(utw) == 1                                                    
                                                                                
    assert m.takes_ptr(utw) == 3                                                
    assert m.takes_ref(utw) == 2                                                
    assert m.takes_ref_wrap(utw) == 2                                           
                                                                                
    assert m.takes_const_ptr(utw) == 5                                          
    assert m.takes_const_ref(utw) == 4                                          
    assert m.takes_const_ref_wrap(utw) == 4

I think that makes it even easier to understand.
The name of the type could be UniqueTagWrapper.

I like this version better as it moves the assertion that matters back into python.

YannickJadoul · 2020-12-15T22:00:39Z

I think that makes it even easier to understand.

Huh, OK. This was my first idea, but then I thought "let's make things more descriptive". Wasn't is_const and is_ref more descriptive of what's being tested, as you usually prefer? But I'm fine with the integer tags, if you prefer.

The name of the type could be UniqueTagWrapper.

I'm just not convinced by this. That name describes the internals, not what's being tested by it?
Also, the class is not wrapping anything or any unique tag; only the caster knows about tags.

laramiel · 2020-12-15T22:59:44Z

I think that it's easier to see that each cast path has a unique result, and the numeric is easier to map back than a pair would be.

YannickJadoul · 2020-12-16T00:14:56Z

I think that it's easier to see that each cast path has a unique result, and the numeric is easier to map back than a pair would be.

That's true, yes, I like that argument!
I just found it funny, because I had opted for the longer, more expressive way of doing things over the first idea of , among other reasons, because I though @rwgk would prefer it and make a good point on how that's better than integers :-) Bad intuition, it seems.
But this is definitely fine, as well! I think there's also other tests doing this, like test_stl.py::test_function_with_string_and_vector_string_arg.

YannickJadoul

Oh, I forgot to approve, it seems.
Thanks for the PR, for holding on during the review, and for fixing all the bikeshedding comments, @laramiel!

YannickJadoul · 2020-12-16T00:18:19Z

include/pybind11/cast.h

-    using subcaster_cast_op_type = typename caster_t::template cast_op_type<type>;
-    static_assert(std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value,
-            "std::reference_wrapper<T> caster requires T to have a caster with an `T &` operator");
+    using reference_t = type&;


This could still be collapsed where it's used, to be shorter and more transparent (which was why I suggested type & instead of add_lvalue_reference<type>::type). But if that's the only thing left to change, I'm fine with this as well, and you don't need to add yet another commits, AFAIC.

YannickJadoul · 2020-12-16T00:31:15Z

Regarding CI: I restarted a few times, but that didn't help. No clue why https://developer.download.nvidia.com/hpc-sdk/nvhpc-20-7-20.7-1.x86_64.rpm is unreachable, and PyPy 2 has been failing for a week or so now (also ignore; #2724 is dealing with disabling PyPy 2 in CI for now).

henryiii · 2020-12-16T01:07:34Z

Please assign a milestone, either 2.6.2 or 2.7.0. Thanks. :)

YannickJadoul · 2020-12-16T01:10:13Z

Is this a fix? I guess it could be considered one. I'm happy to add this into 2.6.2; this shouldn't really be affecting existing code, I would hope. (If not, something went wrong?)

rwgk · 2020-12-16T01:11:39Z

I'll take care of the change log.

YannickJadoul · 2020-12-16T01:15:18Z

I'll take care of the change log.

Do we need a changelog? In principle these should be internal changes, that don't immediately concern users. @henryiii?

@tobiasleibner

* CI: Intel icc/icpc via oneAPI Add testing for Intel icc/icpc via the oneAPI images. Intel oneAPI is in a late beta stage, currently shipping oneAPI beta09 with ICC 20.2. CI: Skip Interpreter Tests for Intel Cannot find how to add this, neiter the package `libc6-dev` nor `intel-oneapi-mkl-devel` help when installed to solve this: ``` -- Looking for C++ include pthread.h -- Looking for C++ include pthread.h - not found CMake Error at /__t/cmake/3.18.4/x64/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message): Could NOT find Threads (missing: Threads_FOUND) Call Stack (most recent call first): /__t/cmake/3.18.4/x64/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:458 (_FPHSA_FAILURE_MESSAGE) /__t/cmake/3.18.4/x64/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindThreads.cmake:234 (FIND_PACKAGE_HANDLE_STANDARD_ARGS) tests/test_embed/CMakeLists.txt:17 (find_package) ``` CI: libc6-dev from GCC for ICC CI: Run bare metal for oneAPI CI: Ubuntu 18.04 for oneAPI CI: Intel +Catch -Eigen CI: CMake from Apt (ICC tests) CI: Replace Intel Py with GCC Py CI: Intel w/o GCC's Eigen CI: ICC with verbose make [Debug] Find core dump tests: use arg{} instead of arg() for Intel tests: adding a few more missing {} fix: sync with @tobiasleibner's branch fix: try ubuntu 20-04 fix: drop exit 1 docs: Apply suggestions from code review Co-authored-by: Tobias Leibner <[email protected]> Workaround for ICC enable_if issues Another workaround for ICC's enable_if issues fix error in previous commit Disable one test for the Intel compiler in C++17 mode Add back one instance of py::arg().noconvert() Add NOLINT to fix clang-tidy check Work around for ICC internal error in PYBIND11_EXPAND_SIDE_EFFECTS in C++17 mode CI: Intel ICC with C++17 docs: pybind11/numpy.h does not require numpy at build time. (#2720) This is nice enough to be mentioned explicitly in the docs. docs: Update warning about Python 3.9.0 UB, now that 3.9.1 has been released (#2719) Adjusting `type_caster<std::reference_wrapper<T>>` to support const/non-const propagation in `cast_op`. (#2705) * Allow type_caster of std::reference_wrapper<T> to be the same as a native reference. Before, both std::reference_wrapper<T> and std::reference_wrapper<const T> would invoke cast_op<type>. This doesn't allow the type_caster<> specialization for T to distinguish reference_wrapper types from value types. After, the type_caster<> specialization invokes cast_op<type&>, which allows reference_wrapper to behave in the same way as a native reference type. * Add tests/examples for std::reference_wrapper<const T> * Add tests which use mutable/immutable variants This test is a chimera; it blends the pybind11 casters with a custom pytype implementation that supports immutable and mutable calls. In order to detect the immutable/mutable state, the cast_op needs to propagate it, even through e.g. std::reference<const T> Note: This is still a work in progress; some things are crashing, which likely means that I have a refcounting bug or something else missing. * Add/finish tests that distinguish const& from & Fixes the bugs in my custom python type implementation, demonstrate test that requires const& and reference_wrapper<const T> being treated differently from Non-const. * Add passing a const to non-const method. * Demonstrate non-const conversion of reference_wrapper in tests. Apply formatting presubmit check. * Fix build errors from presubmit checks. * Try and fix a few more CI errors * More CI fixes. * More CI fixups. * Try and get PyPy to work. * Additional minor fixups. Getting close to CI green. * More ci fixes? * fix clang-tidy warnings from presubmit * fix more clang-tidy warnings * minor comment and consistency cleanups * PyDECREF -> Py_DECREF * copy/move constructors * Resolve codereview comments * more review comment fixes * review comments: remove spurious & * Make the test fail even when the static_assert is commented out. This expands the test_freezable_type_caster a bit by: 1/ adding accessors .is_immutable and .addr to compare identity from python. 2/ Changing the default cast_op of the type_caster<> specialization to return a non-const value. In normal codepaths this is a reasonable default. 3/ adding roundtrip variants to exercise the by reference, by pointer and by reference_wrapper in all call paths. In conjunction with 2/, this demonstrates the failure case of the existing std::reference_wrpper conversion, which now loses const in a similar way that happens when using the default cast_op_type<>. * apply presubmit formatting * Revert inclusion of test_freezable_type_caster There's some concern that this test is a bit unwieldly because of the use of the raw <Python.h> functions. Removing for now. * Add a test that validates const references propagation. This test verifies that cast_op may be used to correctly detect const reference types when used with std::reference_wrapper. * mend * Review comments based changes. 1. std::add_lvalue_reference<type> -> type& 2. Simplify the test a little more; we're never returning the ConstRefCaster type so the class_ definition can be removed. * formatted files again. * Move const_ref_caster test to builtin_casters * Review comments: use cast_op and adjust some comments. * Simplify ConstRefCasted test I like this version better as it moves the assertion that matters back into python. ci: drop pypy2 linux, PGI 20.7, add Python 10 dev (#2724) * ci: drop pypy2 linux, add Python 10 dev * ci: fix mistake * ci: commented-out PGI 20.11, drop 20.7 fix: regression with installed pybind11 overriding local one (#2716) * fix: regression with installed pybind11 overriding discovered one Closes #2709 * docs: wording incorrect style: remove redundant instance->owned = true (#2723) which was just before set to True in instance->allocate_layout() fix: also throw in the move-constructor added by the PYBIND11_OBJECT macro, after the argument has been moved-out (if necessary) (#2701) Make args_are_all_* ICC workarounds unconditional Disable test_aligned on Intel ICC Fix test_aligned on Intel ICC Skip test_python_alreadyset_in_destructor on Intel ICC Fix test_aligned again ICC CI: Downgrade pytest pytest 6 does not capture the `discard_as_unraisable` stderr and just writes a warning with its content instead. * refactor: simpler Intel workaround, suggested by @laramiel * fix: try version with impl to see if it is easier to compile * docs: update README for ICC Co-authored-by: Axel Huebl <[email protected]> Co-authored-by: Henry Schreiner <[email protected]>

laramiel added 2 commits November 30, 2020 09:54

Add tests/examples for std::reference_wrapper<const T>

c603907

rwgk reviewed Dec 1, 2020

View reviewed changes

rwgk requested review from bstaletic and henryiii December 1, 2020 19:15

laramiel added 3 commits December 1, 2020 19:16

Add/finish tests that distinguish const& from &

35e7fa5

Fixes the bugs in my custom python type implementation, demonstrate test that requires const& and reference_wrapper<const T> being treated differently from Non-const.

Add passing a const to non-const method.

4cd06d3

Demonstrate non-const conversion of reference_wrapper in tests.

5abd823

Apply formatting presubmit check.

laramiel added 12 commits December 1, 2020 21:32

Fix build errors from presubmit checks.

c5bd104

Try and fix a few more CI errors

f0e41f5

More CI fixes.

b56a568

More CI fixups.

d71cd7a

Try and get PyPy to work.

3c69635

Additional minor fixups. Getting close to CI green.

8f32ba3

More ci fixes?

d144e59

fix clang-tidy warnings from presubmit

a7ec72e

fix more clang-tidy warnings

e1aca4b

minor comment and consistency cleanups

005d3fe

PyDECREF -> Py_DECREF

4ac23ca

copy/move constructors

a3d9b18

rwgk reviewed Dec 3, 2020

View reviewed changes

Resolve codereview comments

5b4fb98

rwgk approved these changes Dec 3, 2020

View reviewed changes

more review comment fixes

8dcbc42

laramiel added 2 commits December 14, 2020 11:44

Add a test that validates const references propagation.

d869976

This test verifies that cast_op may be used to correctly detect const reference types when used with std::reference_wrapper.

mend

f109edd

bstaletic reviewed Dec 14, 2020

View reviewed changes

laramiel added 4 commits December 14, 2020 13:02

Review comments based changes.

775a7c9

1. std::add_lvalue_reference<type> -> type& 2. Simplify the test a little more; we're never returning the ConstRefCaster type so the class_ definition can be removed.

formatted files again.

6f6dac7

Move const_ref_caster test to builtin_casters

fd6dc5b

Review comments: use cast_op and adjust some comments.

b3ecc53

Simplify ConstRefCasted test

c22772b

I like this version better as it moves the assertion that matters back into python.

YannickJadoul approved these changes Dec 16, 2020

View reviewed changes

rwgk changed the title ~~Allow type_caster of std::reference_wrapper<T> to behave like a native reference.~~ Adjusting type_caster<std::reference_wrapper<T>> to support const/non-const propagation in cast_op. Dec 16, 2020

rwgk merged commit 5469c23 into pybind:master Dec 16, 2020

github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 16, 2020

YannickJadoul added this to the v2.6.2 milestone Dec 16, 2020

rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 16, 2020

changelog for pybind#2705

802e8cc

rwgk mentioned this pull request Dec 16, 2020

changelog for #2705 #2728

Closed

rwgk added casters Related to casters, and to be taken into account when analyzing/reworking casters and removed needs changelog Possibly needs a changelog entry labels Dec 16, 2020

This was referenced Mar 8, 2021

[WIP] Towards a solution for custom-type rvalue arguments #2047

Open

Test type_caster bare interface #2834

Closed

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2705

Closed

		@@ -0,0 +1,408 @@
		/*
		tests/test_chimera.cpp -- This demonstrates a hybrid usage of pybind11, where the

Adjusting type_caster<std::reference_wrapper<T>> to support const/non-const propagation in cast_op. #2705

Adjusting type_caster<std::reference_wrapper<T>> to support const/non-const propagation in cast_op. #2705

Conversation

laramiel commented Nov 30, 2020 • edited Loading

Description

rwgk left a comment

Choose a reason for hiding this comment

laramiel commented Dec 1, 2020 • edited Loading

rwgk commented Dec 1, 2020

laramiel commented Dec 2, 2020

laramiel commented Dec 2, 2020

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YannickJadoul Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Dec 15, 2020

YannickJadoul commented Dec 15, 2020

laramiel commented Dec 15, 2020

YannickJadoul commented Dec 16, 2020

YannickJadoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YannickJadoul commented Dec 16, 2020

henryiii commented Dec 16, 2020

YannickJadoul commented Dec 16, 2020 • edited Loading

rwgk commented Dec 16, 2020

YannickJadoul commented Dec 16, 2020

Adjusting `type_caster<std::reference_wrapper<T>>` to support const/non-const propagation in `cast_op`. #2705

Adjusting `type_caster<std::reference_wrapper<T>>` to support const/non-const propagation in `cast_op`. #2705

laramiel commented Nov 30, 2020 •

edited

Loading

laramiel commented Dec 1, 2020 •

edited

Loading

YannickJadoul Dec 14, 2020 •

edited

Loading

YannickJadoul commented Dec 16, 2020 •

edited

Loading