Re: [PATCH 1/3] Introduce gdb::function_view

[Pedro Alves <palves@redhat.com>]

On 02/22/2017 03:15 PM, Simon Marchi wrote:
>> +
>> +   This is meant to be used as callback type of a function that:
>> +
>> +     #1 - Takes a callback as parameter.
>> +
>> +     #2 - Does not store the callback anywhere; instead if just calls
> 
> if -> it
> 
>> +          it or forwards it to some other function that calls it.

Thanks.  Maybe that's too many confusing "it"s.  I've rewritten it to

     #2 - Does not store the callback anywhere; instead the function
          just calls the callback directly or forwards it to some
          other function that calls it.

>> +
>> +   Usage:
>> +
>> +   Given this function that accepts a callback:
> 
> It's not necessary, but it would be nice to have the equivalent example
> of how it would've been done before (with a function pointer) so that
> people can relate the following example to something they already know.

I wasn't sure whether that should be here, thinking it might
be more appropriate for the internals manual?  I would paste
an example using a callable as template parameter there too, for
example.  But I guess writing it too can't hurt, since that's likely
where people will look first when they want to know what are those
"function_view"s in the source.

I've extended this section in that direction, and tried to clarify
other bits along with it.  See below.

> 
>> +    void
>> +    iterate_over_foos (gdb::function_view<void (foo *)> callback)
>> +    {
>> +       for (auto & : foos)
> 
> I think you're missing a "foo" here.

Indeed.  Found another buglet in the examples that I fixed too.

>> +namespace traits
>> +{
> 
> Is it intended to have this { on a separate line, unlike the other
> namespace declarations?

Nope, somehow missed it.

> I am not going to try to understand any of this...  but as long as it
> works I'm happy.

Yeah, these library facilities tend to require use of C++ that is
not normally used by "regular" code.  In isolation, none of the
features used is really complicated, but when they're all combined,
it gives the impression otherwise.  The advantage is that it
ends up all contained in a single place, while users of the library
end up being simple and efficient.  Let me know if I can explain
things better.

Here's the tweak I mean to squash down addressing your review.
I also realized that the unit test had some formatting issues, and
the symbol names uses there could be renamed to make things a bit
clearer.

I'll send the updated (squashed) patch as a reply.

From 79ce1e9b5fc1e69747d033fc384c7f50c61be1ba Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 22 Feb 2017 17:30:00 +0000
Subject: [PATCH] review

---
 gdb/common/function-view.h              | 104 ++++++++++++++++++++----------
 gdb/unittests/function-view-selftests.c | 109 ++++++++++++++++----------------
 2 files changed, 125 insertions(+), 88 deletions(-)

diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h
index 3303783..af2593f 100644
--- a/gdb/common/function-view.h
+++ b/gdb/common/function-view.h
@@ -30,33 +30,67 @@
 
      #1 - Takes a callback as parameter.
 
-     #2 - Does not store the callback anywhere; instead if just calls
-          it or forwards it to some other function that calls it.
-
-     #3 - When we don't want, or can't make said function be a
-          template function with the callable type as template
-          parameter.  For example, when the callback is a parameter of
-          a virtual member function, or when putting the function
-          template in a header would expose too much implementation
-          detail.
-
-   For this use case, which is quite pervasive, a function_view is a
-   better choice for callback type than std::function.  It is a better
-   choice because std::function is a heavy-weight object with value
+     #2 - Wants to support arbitrary callable objects as callback type
+	  (e.g., stateful function objects, lambda closures, free
+	  functions).
+
+     #3 - Does not store the callback anywhere; instead the function
+	  just calls the callback directly or forwards it to some
+	  other function that calls it.
+
+     #4 - Can't be, or we don't want it to be, a template function
+	  with the callable type as template parameter.  For example,
+	  when the callback is a parameter of a virtual member
+	  function, or when putting the function template in a header
+	  would expose too much implementation detail.
+
+   Note that the C-style "function pointer" + "void *data" callback
+   parameter idiom fails requirement #2 above.  Please don't add new
+   uses of that idiom.  I.e., something like this wouldn't work;
+
+    typedef bool (iterate_over_foos_cb) (foo *f, void *user_data),
+    void iterate_over_foos (iterate_over_foos_cb *callback, void *user_data);
+
+    foo *find_foo_by_type (int type)
+    {
+      foo *found = nullptr;
+
+      iterate_over_foos ([&] (foo *f, void *data)
+	{
+	  if (foo->type == type)
+	    {
+	      found = foo;
+	      return true; // stop iterating
+	    }
+	  return false; // continue iterating
+	}, NULL);
+
+      return found;
+    }
+
+   The above wouldn't compile, because lambdas with captures can't be
+   implicitly converted to a function pointer (because a capture means
+   some context data must be passed to the lambda somehow).
+
+   C++11 gave us std::function as type-erased wrapper around arbitrary
+   callables, however, std::function is not an ideal fit for transient
+   callbacks such as the use case above.  For this use case, which is
+   quite pervasive, a function_view is a better choice, because while
+   while function_view is light and does not require any heap
+   allocation, std::function is a heavy-weight object with value
    semantics that generally requires a heap allocation on
-   construction/assignment of the target callable, while function_view
-   is light and does not require any heap allocation.  It _is_
-   possible to use std::function in such a way that avoids most of the
-   overhead by making sure to only construct it with callables of
-   types that fit std::function's small object optimization, such as
-   function pointers and std::reference_wrapper callables, however,
-   that is quite inconvenient in practice, because restricting to
-   free-function callables would imply no state/capture (which we need
-   in most cases), and std::reference_wrapper implies remembering to
-   use std::ref/std::cref where the callable is constructed, with the
-   added inconvenience that those function have deleted rvalue-ref
-   overloads, meaning you can't use unnamed/temporary lambdas with
-   them.
+   construction/assignment of the target callable.  In addition, while
+   it is possible to use std::function in such a way that avoids most
+   of the overhead by making sure to only construct it with callables
+   of types that fit std::function's small object optimization, such
+   as function pointers and std::reference_wrapper callables, that is
+   quite inconvenient in practice, because restricting to
+   free-function callables would imply no state/capture/closure, which
+   we need in most cases, and std::reference_wrapper implies
+   remembering to use std::ref/std::cref where the callable is
+   constructed, with the added inconvenience that std::ref/std::cref
+   have deleted rvalue-ref overloads, meaning you can't use
+   unnamed/temporary lambdas with them.
 
    Note that because function_view is a non-owning view of a callable,
    care must be taken to ensure that the callable outlives the
@@ -81,15 +115,16 @@
     void
     iterate_over_foos (gdb::function_view<void (foo *)> callback)
     {
-       for (auto & : foos)
-         callback (&foo);
+       for (auto &foo : foos)
+	 callback (&foo);
     }
 
    you can call it like this, passing a lambda as callback:
 
-    iterate_over_foos ([&] (foo *f) {
-      process_one_foo (f);
-    });
+    iterate_over_foos ([&] (foo *f)
+      {
+        process_one_foo (f);
+      });
 
    or like this, passing a function object as callback:
 
@@ -97,15 +132,16 @@
     {
       void operator() (foo *f)
       {
-        if (s->check ())
-          process_one_foo (f);
+	if (s->check ())
+	  process_one_foo (f);
       }
 
       // some state
       state *s;
     };
 
-    function_object matcher (mystate);
+    state mystate;
+    function_object matcher {&mystate};
     iterate_over_foos (matcher);
 
   or like this, passing a function pointer as callback:
diff --git a/gdb/unittests/function-view-selftests.c b/gdb/unittests/function-view-selftests.c
index 8f73bc4..3e5369b 100644
--- a/gdb/unittests/function-view-selftests.c
+++ b/gdb/unittests/function-view-selftests.c
@@ -27,143 +27,144 @@ namespace selftests {
 namespace function_view {
 
 static int
-add_one (int count)
+plus_one_fn_int (int val)
 {
-  return ++count;
+  return ++val;
 }
 
 static short
-add_one_short (short count)
+plus_one_fn_short (short val)
 {
-  return ++count;
+  return ++val;
 }
 
 static int
-call_callback (int count, gdb::function_view <int(int)> callback)
+call_callback_int (int val, gdb::function_view <int (int)> callback)
 {
-  return callback (count);
+  return callback (val);
 }
 
 static void
-call_callback_void (int count, gdb::function_view <void(int)> callback)
+call_callback_void (int val, gdb::function_view <void (int)> callback)
 {
-  callback (count);
+  callback (val);
 }
 
-struct plus_one_function_object
+struct plus_one_int_func_obj
 {
-  int operator () (int count)
+  int operator () (int val)
   {
-    ++calls;
-    return ++count;
+    ++call_count;
+    return ++val;
   }
 
-  int calls = 0;
+  /* Number of times called.  */
+  int call_count = 0;
 };
 
 static void
 run_tests ()
 {
   /* A simple lambda.  */
-  auto plus_one = [] (int count) { return ++count; };
+  auto plus_one_lambda = [] (int val) { return ++val; };
 
   /* A function_view that references the lambda.  */
-  gdb::function_view<int (int)> plus_one_view (plus_one);
+  gdb::function_view<int (int)> plus_one_func_view (plus_one_lambda);
 
   /* Check calling the lambda directly.  */
-  SELF_CHECK (plus_one (0) == 1);
-  SELF_CHECK (plus_one (1) == 2);
+  SELF_CHECK (plus_one_lambda (0) == 1);
+  SELF_CHECK (plus_one_lambda (1) == 2);
 
   /* Check calling lambda via the view.  */
-  SELF_CHECK (plus_one_view (2) == 3);
-  SELF_CHECK (plus_one_view (3) == 4);
+  SELF_CHECK (plus_one_func_view (2) == 3);
+  SELF_CHECK (plus_one_func_view (3) == 4);
 
   /* Check calling a function that takes a function_view as argument,
      by value.  Pass a lambda, making sure a function_view is properly
      constructed implicitly.  */
-  SELF_CHECK (call_callback (1, [] (int count)
+  SELF_CHECK (call_callback_int (1, [] (int val)
     {
-      return count + 2;
+      return val + 2;
     }) == 3);
 
   /* Same, passing a named/lvalue lambda.  */
-  SELF_CHECK (call_callback (1, plus_one) == 2);
+  SELF_CHECK (call_callback_int (1, plus_one_lambda) == 2);
   /* Same, passing named/lvalue function_view (should copy).  */
-  SELF_CHECK (call_callback (1, plus_one_view) == 2);
+  SELF_CHECK (call_callback_int (1, plus_one_func_view) == 2);
 
   /* Check constructing a function view over a function-object
      callable, and calling it.  */
-  plus_one_function_object func_obj;
+  plus_one_int_func_obj func_obj;
   SELF_CHECK (func_obj (0) == 1);
-  SELF_CHECK (call_callback (1, func_obj) == 2);
+  SELF_CHECK (call_callback_int (1, func_obj) == 2);
   /* Check that the callable was referenced, not copied.  */
-  SELF_CHECK (func_obj.calls == 2);
+  SELF_CHECK (func_obj.call_count == 2);
 
-  /* Check constructing a function_view over free-function callable,
+  /* Check constructing a function_view over a free-function callable,
      and calling it.  */
-  SELF_CHECK (call_callback (1, add_one) == 2);
+  SELF_CHECK (call_callback_int (1, plus_one_fn_int) == 2);
 
   /* Check calling a function with a
      compatible-but-not-exactly-the-same prototype.  */
-  SELF_CHECK (call_callback (1, [] (short count) -> short
+  SELF_CHECK (call_callback_int (1, [] (short val) -> short
     {
-      return count + 2;
+      return val + 2;
     }) == 3);
   /* Same, but passing a function pointer.  */
-  SELF_CHECK (call_callback (1, add_one_short) == 2);
+  SELF_CHECK (call_callback_int (1, plus_one_fn_short) == 2);
 
   /* Like std::function, a function_view that expects a void return
      can reference callables with non-void return type.  The result is
      simply discarded.  Check a lambda, function object and a function
      pointer.  */
-  call_callback_void (1, [] (int count) -> int
+  call_callback_void (1, [] (int val) -> int
     {
-      return count + 2;
+      return val + 2;
     });
   call_callback_void (1, func_obj);
-  call_callback_void (1, add_one);
+  call_callback_void (1, plus_one_fn_int);
 
   /* Check that the main ctor doesn't hijack the copy ctor.  */
-  auto plus_one_view2 (plus_one_view);
-  auto plus_one_view3 (plus_one_view2);
-  static_assert (std::is_same<decltype (plus_one_view),
-		 decltype (plus_one_view2)>::value, "");
-  static_assert (std::is_same<decltype (plus_one_view),
-		 decltype (plus_one_view3)>::value, "");
+  auto plus_one_func_view2 (plus_one_func_view);
+  auto plus_one_func_view3 (plus_one_func_view2);
+  static_assert (std::is_same<decltype (plus_one_func_view),
+		 decltype (plus_one_func_view2)>::value, "");
+  static_assert (std::is_same<decltype (plus_one_func_view),
+		 decltype (plus_one_func_view3)>::value, "");
 
-  SELF_CHECK (plus_one_view3 (1) == 2);
+  SELF_CHECK (plus_one_func_view3 (1) == 2);
 
   /* Likewise, but propagate a NULL callable.  If this calls the main
      function_view ctor instead of the copy ctor by mistake, then
      null_func_2 ends up non-NULL (because it'd instead reference
      null_func_1 as just another callable).  */
-  constexpr gdb::function_view<int(int)> null_func_1 = nullptr;
-  constexpr auto null_func_2 (null_func_1);
+  constexpr gdb::function_view<int (int)> null_func_view_1 = nullptr;
+  constexpr auto null_func_view_2 (null_func_view_1);
 
   /* While at it, check whether the function_view is bound using
      various forms, op==, op!= and op bool.  */
 
   /* op== */
-  static_assert (null_func_2 == nullptr, "");
-  static_assert (nullptr == null_func_2, "");
-  static_assert (null_func_2 == NULL, "");
-  static_assert (NULL == null_func_2, "");
+  static_assert (null_func_view_2 == nullptr, "");
+  static_assert (nullptr == null_func_view_2, "");
+  static_assert (null_func_view_2 == NULL, "");
+  static_assert (NULL == null_func_view_2, "");
 
   /* op!= */
-  static_assert (!(null_func_2 != nullptr), "");
-  static_assert (!(nullptr != null_func_2), "");
-  static_assert (!(null_func_2 != NULL), "");
-  static_assert (!(NULL != null_func_2), "");
+  static_assert (!(null_func_view_2 != nullptr), "");
+  static_assert (!(nullptr != null_func_view_2), "");
+  static_assert (!(null_func_view_2 != NULL), "");
+  static_assert (!(NULL != null_func_view_2), "");
 
   /* op bool */
-  static_assert (!null_func_2, "");
+  static_assert (!null_func_view_2, "");
 
   /* Check the nullptr_t ctor.  */
-  constexpr gdb::function_view<int(int)> check_ctor_nullptr (nullptr);
+  constexpr gdb::function_view<int (int)> check_ctor_nullptr (nullptr);
   static_assert (!check_ctor_nullptr, "");
 
   /* Check the nullptr_t op= */
-  gdb::function_view<int(int)> check_op_eq_null (add_one);
+  gdb::function_view<int (int)> check_op_eq_null (plus_one_fn_int);
   SELF_CHECK (check_op_eq_null);
   check_op_eq_null = nullptr;
   SELF_CHECK (!check_op_eq_null);
-- 
2.5.5