From patchwork Wed Feb 22 17:40:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 19342 Received: (qmail 116367 invoked by alias); 22 Feb 2017 17:40:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 113824 invoked by uid 89); 22 Feb 2017 17:40:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=realized, Wants X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Feb 2017 17:40:48 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 847B88047D; Wed, 22 Feb 2017 17:40:48 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1MHekVT032402; Wed, 22 Feb 2017 12:40:47 -0500 Subject: Re: [PATCH 1/3] Introduce gdb::function_view To: Simon Marchi References: <1487775033-32699-1-git-send-email-palves@redhat.com> <1487775033-32699-2-git-send-email-palves@redhat.com> <8636f39b5e1e2cbdeabbf8dfd999e709@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <4c8528d8-0115-3bf3-0a16-42f44be580a9@redhat.com> Date: Wed, 22 Feb 2017 17:40:45 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <8636f39b5e1e2cbdeabbf8dfd999e709@polymtl.ca> 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 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::value, ""); - static_assert (std::is_same::value, ""); + auto plus_one_func_view2 (plus_one_func_view); + auto plus_one_func_view3 (plus_one_func_view2); + static_assert (std::is_same::value, ""); + static_assert (std::is_same::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);