Allow slots with rvalue reference parameters

slavaandrejev · kjellahl · commit 7d85510b1011 · 2021-06-02T21:29:39.000+02:00
In code based on Glibmm it's quite common to convert a Glib C structure
to a Glib::Object's descendant before passing it as a parameter to a
slot. If the slot is not supposed to modify the object, then everything
is fine, we can declare the parameter as a const reference and the
temporary object will be accurately dealt with by the compiler. However,
if the Glib based object is getting modified, the only way to pass it to
the slot is to declare it a Glib::RefPtr shared pointer. This creates a
lot of unnecessary churn of memory allocations for a simple deal of
calling a signal. However, C++ has a specific mean for this particular
semantic of passing a temporary object: rvalue reference.

For example, somewhere inside Gtkmm might be a declaration:
_WRAP_SIGNAL(void sun_rose(const Glib::RefPtr&lt;Gtk::Sun&gt; &amp;new_sun),
"sun-rose")

Instead its more semantically correct to write:
_WRAP_SIGNAL(void sun_rose(Gtk::Sun &amp;&amp;new_sun), "sun-rose")

And later somewhere in your code:
world-&gt;signal_sun_rose().connect([&amp;](auto &amp;&amp;new_sun) {
  new_sun.shine();
});

This commit makes a couple of simple modifications that allow declaring
signals and slots with rvalue reference parameter.
diff --git a/sigc++/functors/slot.h b/sigc++/functors/slot.h
@@ -151,7 +151,8 @@ struct slot_call
   static T_return call_it(slot_rep* rep, type_trait_take_t<T_arg>... a_)
   {
     auto typed_rep = static_cast<typed_slot_rep<T_functor>*>(rep);
-    return (*typed_rep->functor_).template operator()<type_trait_take_t<T_arg>...>(a_...);
+    return (*typed_rep->functor_).template operator()<type_trait_take_t<T_arg>...>(
+      std::forward<type_trait_take_t<T_arg>>(a_)...);
   }
 
   /** Forms a function pointer from call_it().
@@ -220,7 +221,7 @@ class slot<T_return(T_arg...)> : public slot_base
     {
       return std::invoke(sigc::internal::function_pointer_cast<call_type>(slot_base::rep_->call_),
         slot_base::rep_,
-        a...);
+        std::forward<type_trait_take_t<T_arg>>(a)...);
     }
 
     return T_return();
diff --git a/sigc++/signal.h b/sigc++/signal.h
@@ -363,7 +363,9 @@ struct signal_emit<void, void, T_arg...>
       if (slot.empty() || slot.blocked())
         continue;
 
-      (sigc::internal::function_pointer_cast<call_type>(slot.rep_->call_))(slot.rep_, a...);
+      (sigc::internal::function_pointer_cast<call_type>(slot.rep_->call_))(
+        slot.rep_,
+        std::forward<type_trait_take_t<T_arg>>(a)...);
     }
   }
 };
@@ -450,11 +452,13 @@ class signal_with_accumulator : public signal_base
   decltype(auto) emit(type_trait_take_t<T_arg>... a) const
   {
     using emitter_type = internal::signal_emit<T_return, T_accumulator, T_arg...>;
-    return emitter_type::emit(impl_, a...);
+    return emitter_type::emit(impl_, std::forward<type_trait_take_t<T_arg>>(a)...);
   }
 
   /** Triggers the emission of the signal (see emit()). */
-  decltype(auto) operator()(type_trait_take_t<T_arg>... a) const { return emit(a...); }
+  decltype(auto) operator()(type_trait_take_t<T_arg>... a) const {
+    return emit(std::forward<type_trait_take_t<T_arg>>(a)...);
+  }
 
   /** Creates a functor that calls emit() on this signal.
    * @code
diff --git a/sigc++/type_traits.h b/sigc++/type_traits.h
@@ -52,6 +52,13 @@ struct type_trait<const T_type&>
   using take = const T_type&;
 };
 
+template<typename T_type>
+struct type_trait<T_type&&>
+{
+  using pass = T_type&&;
+  using take = T_type&&;
+};
+
 template<>
 struct type_trait<void>
 {
diff --git a/tests/memleakcheck.sh b/tests/memleakcheck.sh
@@ -9,8 +9,8 @@ for testprog in  test_accum_iter test_accumulated test_bind test_bind_as_slot \
   test_copy_invalid_slot test_cpp11_lambda test_custom test_disconnect \
   test_disconnect_during_emit test_exception_catch test_hide \
   test_limit_reference test_member_method_trait test_mem_fun test_ptr_fun \
-  test_retype test_retype_return test_signal test_signal_move test_size \
-  test_slot test_slot_disconnect test_slot_move test_trackable \
+  test_retype test_retype_return test_rvalue_ref test_signal test_signal_move \
+  test_size test_slot test_slot_disconnect test_slot_move test_trackable \
   test_trackable_move test_track_obj test_tuple_cdr test_tuple_end \
   test_tuple_for_each test_tuple_start test_tuple_transform_each \
   test_visit_each test_visit_each_trackable test_weak_raw_ptr
diff --git a/tests/meson.build b/tests/meson.build
@@ -28,6 +28,7 @@ test_programs = [
   [[], 'test_ptr_fun', ['test_ptr_fun.cc', 'testutilities.cc']],
   [[], 'test_retype', ['test_retype.cc', 'testutilities.cc']],
   [[], 'test_retype_return', ['test_retype_return.cc', 'testutilities.cc']],
+  [[], 'test_rvalue_ref', ['test_rvalue_ref.cc', 'testutilities.cc']],
   [[], 'test_signal', ['test_signal.cc', 'testutilities.cc']],
   [[], 'test_signal_move', ['test_signal_move.cc', 'testutilities.cc']],
   [[], 'test_size', ['test_size.cc', 'testutilities.cc']],
diff --git a/tests/test_rvalue_ref.cc b/tests/test_rvalue_ref.cc
@@ -0,0 +1,55 @@
+#include "testutilities.h"
+#include <iostream>
+#include <sigc++/signal.h>
+
+struct MoveableStruct {};
+
+namespace
+{
+TestUtilities* util = nullptr;
+std::ostringstream result_stream;
+
+struct foo
+{
+  void operator()(MoveableStruct &&x)
+  {
+    result_stream << "foo(MoveableStruct&&)";
+  }
+};
+
+} // end anonymous namespace
+
+void
+test_signal()
+{
+  sigc::signal<void (MoveableStruct &&)> signal;
+  foo f;
+  signal.connect(f);
+  MoveableStruct x;
+  signal(std::move(x));
+  util->check_result(result_stream, "foo(MoveableStruct&&)");
+}
+
+void
+test_slot()
+{
+  sigc::slot<void (MoveableStruct &&)> slot;
+  foo f;
+  slot = f;
+  MoveableStruct x;
+  slot(std::move(x));
+  util->check_result(result_stream, "foo(MoveableStruct&&)");
+}
+
+int
+main(int argc, char* argv[])
+{
+  util = TestUtilities::get_instance();
+  if (!util->check_command_args(argc, argv))
+    return util->get_result_and_delete_instance() ? EXIT_SUCCESS : EXIT_FAILURE;
+
+  test_signal();
+  test_slot();
+
+  return util->get_result_and_delete_instance() ? EXIT_SUCCESS : EXIT_FAILURE;
+} // end main()

Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,8 @@ struct slot_call`
`151`	`151`	`static T_return call_it(slot_rep* rep, type_trait_take_t<T_arg>... a_)`
`152`	`152`	`{`
`153`	`153`	`auto typed_rep = static_cast<typed_slot_rep<T_functor>*>(rep);`
`154`		`- return (*typed_rep->functor_).template operator()<type_trait_take_t<T_arg>...>(a_...);`
	`154`	`+ return (*typed_rep->functor_).template operator()<type_trait_take_t<T_arg>...>(`
	`155`	`+ std::forward<type_trait_take_t<T_arg>>(a_)...);`
`155`	`156`	`}`
`156`	`157`
`157`	`158`	`/** Forms a function pointer from call_it().`
`@@ -220,7 +221,7 @@ class slot<T_return(T_arg...)> : public slot_base`
`220`	`221`	`{`
`221`	`222`	`return std::invoke(sigc::internal::function_pointer_cast<call_type>(slot_base::rep_->call_),`
`222`	`223`	`slot_base::rep_,`
`223`		`- a...);`
	`224`	`+ std::forward<type_trait_take_t<T_arg>>(a)...);`
`224`	`225`	`}`
`225`	`226`
`226`	`227`	`return T_return();`
Original file line number	Diff line number	Diff line change
`@@ -363,7 +363,9 @@ struct signal_emit<void, void, T_arg...>`
`363`	`363`	`if (slot.empty() \|\| slot.blocked())`
`364`	`364`	`continue;`
`365`	`365`
`366`		`- (sigc::internal::function_pointer_cast<call_type>(slot.rep_->call_))(slot.rep_, a...);`
	`366`	`+ (sigc::internal::function_pointer_cast<call_type>(slot.rep_->call_))(`
	`367`	`+ slot.rep_,`
	`368`	`+ std::forward<type_trait_take_t<T_arg>>(a)...);`
`367`	`369`	`}`
`368`	`370`	`}`
`369`	`371`	`};`
`@@ -450,11 +452,13 @@ class signal_with_accumulator : public signal_base`
`450`	`452`	`decltype(auto) emit(type_trait_take_t<T_arg>... a) const`
`451`	`453`	`{`
`452`	`454`	`using emitter_type = internal::signal_emit<T_return, T_accumulator, T_arg...>;`
`453`		`- return emitter_type::emit(impl_, a...);`
	`455`	`+ return emitter_type::emit(impl_, std::forward<type_trait_take_t<T_arg>>(a)...);`
`454`	`456`	`}`
`455`	`457`
`456`	`458`	`/** Triggers the emission of the signal (see emit()). */`
`457`		`- decltype(auto) operator()(type_trait_take_t<T_arg>... a) const { return emit(a...); }`
	`459`	`+ decltype(auto) operator()(type_trait_take_t<T_arg>... a) const {`
	`460`	`+ return emit(std::forward<type_trait_take_t<T_arg>>(a)...);`
	`461`	`+ }`
`458`	`462`
`459`	`463`	`/** Creates a functor that calls emit() on this signal.`
`460`	`464`	`* @code`