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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-02-22 12:40, Pedro Alves wrote:
> 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.

LGTM.

>>> +
>>> +   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);

That's fine with me.