From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24681 invoked by alias); 22 Feb 2017 18:02:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 24663 invoked by uid 89); 22 Feb 2017 18:02:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Feb 2017 18:02:10 +0000 Received: by simark.ca (Postfix, from userid 33) id 7BA311E7DE; Wed, 22 Feb 2017 13:02:09 -0500 (EST) To: Pedro Alves Subject: Re: [PATCH 1/3] Introduce gdb::function_view X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 22 Feb 2017 18:02:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <4c8528d8-0115-3bf3-0a16-42f44be580a9@redhat.com> References: <1487775033-32699-1-git-send-email-palves@redhat.com> <1487775033-32699-2-git-send-email-palves@redhat.com> <8636f39b5e1e2cbdeabbf8dfd999e709@polymtl.ca> <4c8528d8-0115-3bf3-0a16-42f44be580a9@redhat.com> Message-ID: <61b1e228e36f3fbb0d8bd07432622e45@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00599.txt.bz2 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 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 > 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 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 callback) > +call_callback_int (int val, gdb::function_view callback) > { > - return callback (count); > + return callback (val); > } > > static void > -call_callback_void (int count, gdb::function_view > callback) > +call_callback_void (int val, gdb::function_view 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 plus_one_view (plus_one); > + gdb::function_view 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_view2)>::value, ""); > - static_assert (std::is_same - 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_view2)>::value, ""); > + static_assert (std::is_same + 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 null_func_1 = nullptr; > - constexpr auto null_func_2 (null_func_1); > + constexpr gdb::function_view 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 check_ctor_nullptr (nullptr); > + constexpr gdb::function_view check_ctor_nullptr > (nullptr); > static_assert (!check_ctor_nullptr, ""); > > /* Check the nullptr_t op= */ > - gdb::function_view check_op_eq_null (add_one); > + gdb::function_view 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.