From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85317 invoked by alias); 22 Feb 2017 15:15:50 -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 85299 invoked by uid 89); 22 Feb 2017 15:15:49 -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 15:15:45 +0000 Received: by simark.ca (Postfix, from userid 33) id D625C1E7DE; Wed, 22 Feb 2017 10:15:43 -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 15:15:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1487775033-32699-2-git-send-email-palves@redhat.com> References: <1487775033-32699-1-git-send-email-palves@redhat.com> <1487775033-32699-2-git-send-email-palves@redhat.com> Message-ID: <8636f39b5e1e2cbdeabbf8dfd999e709@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00595.txt.bz2 On 2017-02-22 09:50, Pedro Alves wrote: > This commit adds new function_view type. This type holds a a > non-owning reference to a callable. It is meant to be used as > callback type of functions, instead of using C-style pair of function > pointer and 'void *data' arguments. function_view allows passing > references to stateful function objects / lambdas w/ captures as > callbacks efficiently, while function pointer + 'void *' does not. > > See the intro in the new function-view.h header for more. > > Unit tests included. I added a new gdb/unittests/ subdir this time, > instead of putting the tests under gdb/. If this is agreed to be a > good idea, some of the current selftests that exercise gdb/common/ > things but live in gdb/ could move here (e.g., gdb/utils-selftests.c). > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves > > * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. > (SFILES): Add $(SUBDIR_UNITTEST_SRCS). > (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). > (%.o) : New pattern. > (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). > * common/function-view.h: New file. > * unittests/function-view-selftests.c: New file. > --- > gdb/Makefile.in | 24 ++- > gdb/common/function-view.h | 320 > ++++++++++++++++++++++++++++++++ > gdb/unittests/function-view-selftests.c | 183 ++++++++++++++++++ > 3 files changed, 524 insertions(+), 3 deletions(-) > create mode 100644 gdb/common/function-view.h > create mode 100644 gdb/unittests/function-view-selftests.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 43253d3..a4cac36 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -523,6 +523,12 @@ SUBDIR_PYTHON_DEPS = > SUBDIR_PYTHON_LDFLAGS = > SUBDIR_PYTHON_CFLAGS = > > +SUBDIR_UNITTESTS_SRCS = \ > + unittests/function-view-selftests.c > + > +SUBDIR_UNITTESTS_OBS = \ > + function-view-selftests.o > + > # Opcodes currently live in one of two places. Either they are in the > # opcode library, typically ../opcodes, or they are in a header file > # in INCLUDE_DIR. > @@ -1216,7 +1222,8 @@ SFILES = \ > common/xml-utils.c \ > mi/mi-common.c \ > target/waitstatus.c \ > - $(SUBDIR_GCC_COMPILE_SRCS) > + $(SUBDIR_GCC_COMPILE_SRCS) \ > + $(SUBDIR_UNITTEST_SRCS) > > LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c > > @@ -1800,7 +1807,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ > xml-syscall.o \ > xml-tdesc.o \ > xml-utils.o \ > - $(SUBDIR_GCC_COMPILE_OBS) > + $(SUBDIR_GCC_COMPILE_OBS) \ > + $(SUBDIR_UNITTESTS_OBS) > > TSOBS = inflow.o > > @@ -1909,6 +1917,10 @@ all: gdb$(EXEEXT) $(CONFIG_ALL) > $(COMPILE) $< > $(POSTCOMPILE) > > +%.o: ${srcdir}/unittests/%.c > + $(COMPILE) $< > + $(POSTCOMPILE) > + > # Specify an explicit rule for gdb/common/agent.c, to avoid a clash > with the > # object file generate by gdb/agent.c. > common-agent.o: $(srcdir)/common/agent.c > @@ -2124,7 +2136,13 @@ test-cp-name-parser$(EXEEXT): > test-cp-name-parser.o $(LIBIBERTY) > # duplicates. Files in the gdb/ directory can end up appearing in > # COMMON_OBS (as a .o file) and CONFIG_SRCS (as a .c file). > > -INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS) > $(SUBDIR_GCC_COMPILE_SRCS) > +INIT_FILES = \ > + $(COMMON_OBS) \ > + $(TSOBS) \ > + $(CONFIG_SRCS) \ > + $(SUBDIR_GCC_COMPILE_SRCS) \ > + $(SUBDIR_UNITTESTS_SRCS) > + > init.c: $(INIT_FILES) > @echo Making init.c > @rm -f init.c-tmp init.l-tmp > diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h > new file mode 100644 > index 0000000..cd455f8 > --- /dev/null > +++ b/gdb/common/function-view.h > @@ -0,0 +1,320 @@ > +/* Copyright (C) 2017 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or > modify > + it under the terms of the GNU General Public License as published > by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > . */ > + > +#ifndef COMMON_FUNCTION_VIEW_H > +#define COMMON_FUNCTION_VIEW_H > + > +/* function_view is a polymorphic type-erasing wrapper class that > + encapsulates a non-owning reference to arbitrary callable objects. > + > + A way to put it is that function_view is to std::function like > + std::string_view is to std::string. While std::function stores a > + type-erased callable object internally, function_view holds a > + type-erased reference to an external callable object. > + > + 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. > + > + #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 > + 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. > + > + Note that because function_view is a non-owning view of a callable, > + care must be taken to ensure that the callable outlives the > + function_view that calls it. This is not really a problem for the > + use case function_view is intended for, such as passing a temporary > + function object / lambda to a function that accepts a callback, > + because in those cases, the temporary is guaranteed to be live > + until the called function returns. > + > + Calling a function_view with no associated target is undefined, > + unlike with std::function, which throws bad_function_call. This is > + by design, to avoid the otherwise necessary NULL check in > + function_view::operator(). > + > + Since function_view objects are small (a pair of pointers), they > + should generally be passed around by value. > + > + 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. > + void > + iterate_over_foos (gdb::function_view callback) > + { > + for (auto & : foos) I think you're missing a "foo" here. > + callback (&foo); > + } > + > + you can call it like this, passing a lambda as callback: > + > + iterate_over_foos ([&] (foo *f) { > + process_one_foo (f); > + }); > + > + or like this, passing a function object as callback: > + > + struct function_object > + { > + void operator() (foo *f) > + { > + if (s->check ()) > + process_one_foo (f); > + } > + > + // some state > + state *s; > + }; > + > + function_object matcher (mystate); > + iterate_over_foos (matcher); > + > + or like this, passing a function pointer as callback: > + > + iterate_over_foos (process_one_foo); > + > + You can find unit tests covering the whole API in > + unittests/function-view-selftests.c. */ > + > +namespace gdb { > + > +namespace traits > +{ Is it intended to have this { on a separate line, unlike the other namespace declarations? > + /* A few trait helpers. */ > + template > + struct Not : public std::integral_constant > + {}; > + > + template > + struct Or; > + > + template<> > + struct Or<> : public std::false_type > + {}; > + > + template > + struct Or : public B1 > + {}; > + > + template > + struct Or > + : public std::conditional::type > + {}; > + > + template > + struct Or > + : public std::conditional>::type > + {}; > +} > + > +namespace fv_detail { > +/* Bits shared by all function_view instantiations that do not depend > + on the template parameters. */ > + > +/* Storage for the erased callable. This is a union in order to be > + able to save both a function object (data) pointer or a function > + pointer without triggering undefined behavior. */ > +union erased_callable > +{ > + /* For function objects. */ > + void *data; > + > + /* For function pointers. */ > + void (*fn) (); > +}; > + > +} /* namespace fv_detail */ > + > +/* Use partial specialization to get access to the callable's > + signature. */ > +template > +struct function_view; > + > +template > +class function_view > +{ > + template > + using CompatibleReturnType > + = traits::Or, > + std::is_same, > + std::is_convertible>; > + > + /* True if Func can be called with Args, and the result, and the > + result is convertible to Res, unless Res is void. */ > + template + typename Res2 = typename std::result_of &(Args...)>::type> > + struct IsCompatibleCallable : CompatibleReturnType > + {}; > + > + /* True if Callable is a function_view. Used to avoid hijacking the > + copy ctor. */ > + template > + struct IsFunctionView > + : std::is_same::type> > + {}; > + > + /* Helper to make SFINAE logic easier to read. */ > + template > + using Requires = typename std::enable_if void>::type; > + > + public: > + > + /* NULL by default. */ > + constexpr function_view () noexcept > + : m_erased_callable {}, > + m_invoker {} > + {} > + > + /* Default copy/assignment is fine. */ > + function_view (const function_view &) = default; > + function_view &operator= (const function_view &) = default; > + > + /* This is the main entry point. Use SFINAE to avoid hijacking the > + copy constructor and to ensure that the target type is > + compatible. */ > + template > + + typename = Requires>>, > + typename = Requires>> > + function_view (Callable &&callable) noexcept > + { > + bind (callable); > + } > + > + /* Construct a NULL function_view. */ > + constexpr function_view (std::nullptr_t) noexcept > + : m_erased_callable {}, > + m_invoker {} > + {} > + > + /* Clear a function_view. */ > + function_view &operator= (std::nullptr_t) noexcept > + { > + m_invoker = nullptr; > + return *this; > + } > + > + /* Return true if the wrapper has a target, false otherwise. Note > + we check M_INVOKER instead of M_ERASED_CALLABLE because we don't > + know which member of the union is active right now. */ > + constexpr explicit operator bool () const noexcept > + { return m_invoker != nullptr; } > + > + /* Call the callable. */ > + Res operator () (Args... args) const > + { return m_invoker (m_erased_callable, std::forward > (args)...); } > + > + private: > + > + /* Bind this function_view to a compatible function object > + reference. */ > + template > + void bind (Callable &callable) noexcept > + { > + m_erased_callable.data = (void *) std::addressof (callable); > + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) > + noexcept (noexcept (callable (std::forward (args)...))) -> > Res > + { > + auto &restored_callable = *static_cast (ecall.data); > + /* The explicit cast to Res avoids a compile error when Res is > + void and the callable returns non-void. */ > + return (Res) restored_callable (std::forward (args)...); > + }; > + } > + > + /* Bind this function_view to a compatible function pointer. > + > + Making this a separate function allows avoiding one indirection, > + by storing the function pointer directly in the storage, instead > + of a pointer to pointer. erased_callable is then a union in > + order to avoid storing a function pointer as a data pointer here, > + which would be undefined. */ > + template > + void bind (Res2 (*fn) (Args2...)) noexcept > + { > + m_erased_callable.fn = reinterpret_cast (fn); > + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) > + noexcept (noexcept (fn (std::forward (args)...))) -> Res > + { > + auto restored_fn = reinterpret_cast (ecall.fn); > + /* The explicit cast to Res avoids a compile error when Res is > + void and the callable returns non-void. */ > + return (Res) restored_fn (std::forward (args)...); > + }; > + } > + > + /* Storage for the erased callable. */ > + fv_detail::erased_callable m_erased_callable; > + > + /* The invoker. This is set to a capture-less lambda by one of the > + 'bind' overloads. The lambda restores the right type of the > + callable (which is passed as first argument), and forwards the > + args. */ > + Res (*m_invoker) (fv_detail::erased_callable, Args...); > +}; > + > +/* Allow comparison with NULL. Defer the work to the in-class > + operator bool implementation. */ > + > +template > +constexpr inline bool > +operator== (const function_view &f, std::nullptr_t) > noexcept > +{ return !static_cast (f); } > + > +template > +constexpr inline bool > +operator== (std::nullptr_t, const function_view &f) > noexcept > +{ return !static_cast (f); } > + > +template > +constexpr inline bool > +operator!= (const function_view &f, std::nullptr_t) > noexcept > +{ return static_cast (f); } > + > +template > +constexpr inline bool > +operator!= (std::nullptr_t, const function_view &f) > noexcept > +{ return static_cast (f); } > + > +} /* namespace gdb */ > + > +#endif I am not going to try to understand any of this... but as long as it works I'm happy. Simon