From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2294 invoked by alias); 15 Jan 2019 09:42:04 -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 2285 invoked by uid 89); 15 Jan 2019 09:42:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=replying, hoped, bespoke, horrible X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 15 Jan 2019 09:42:01 +0000 Received: by mail-wr1-f66.google.com with SMTP id v13so2142999wrw.5 for ; Tue, 15 Jan 2019 01:42:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=jt1emzBKhBO268qqwk873979IIE1qFbChToPZbVRCMs=; b=U0v83ZYrNMM15uiR8H4vYLh69L8c8gHteqTMmXmOJd5indjPPPizTZWsE7Sljb2j91 5ZT8JpWUYF34vAEKY3Y6dA5HqZQDKw9BSJ5w+ISXGZD3Q1EeBbgYVD6op8mHJ+T27uyh /v69s81YEy1IebsOP/NI3iR2NgDhEtW4kiWhJ/kEMEUoQcq+jmaplxquAr7fQNnqi8em JhUAclM0FgOTLhzI6PCWPyO7rJpQLN/JMO7bAJqNSW3LoQX3c1/PnF6mOIisbWPz8BNc KstRxJ111MAktqgFJtjAD60CVrOQXTp2zmlca0rBZC7pixRFQe6eTx6kFOulsfLn2kSy T9QQ== Return-Path: Received: from localhost (host86-172-198-47.range86-172.btcentralplus.com. [86.172.198.47]) by smtp.gmail.com with ESMTPSA id a132sm39730714wmh.5.2019.01.15.01.41.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 15 Jan 2019 01:41:58 -0800 (PST) Date: Tue, 15 Jan 2019 09:42:00 -0000 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCH 00/12] remove some cleanups using a cleanup function Message-ID: <20190115094157.GP3456@embecosm.com> References: <20190109033426.16062-1-tom@tromey.com> <961d9501-23e6-9adb-a11b-da41702c4fa0@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <961d9501-23e6-9adb-a11b-da41702c4fa0@redhat.com> X-Fortune: The speed of anything depends on the flow of everything. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00343.txt.bz2 * Pedro Alves [2019-01-14 20:37:41 +0000]: > On 01/12/2019 11:50 AM, Andrew Burgess wrote: > > I've been thinking about a similar idea for a while too. I wondered > > if there was a way we could make use of templates to generate some of > > the common boiler plate cleanups. > > > > This mini-series changes the first 4 of your patches to you this idea > > so you can see how it might work. > > > > First, the ideal, description, a new templated class > > `cleanup_function` that allows you to do something like this: > > > > void > > delete_longjmp_breakpoint (int arg) > > { > > /* Blah, blah, blah... */ > > } > > > > using longjmp_breakpoint_cleanup > > = cleanup_function ; > > It seems unnecessary to pass in the types of the arguments of > delete_longjmp_breakpoint. Couldn't those be extracted from > delete_longjmp_breakpoint's type? See below. > > > > > This creates a new cleanup class `longjmp_breakpoint_cleanup` than can > > then be used like this: > > > > longjmp_breakpoint_cleanup obj (thread); > > I think this results in an inferior cleanup_function solution, because this > way you can't pass in a bespoke small lambda on the spot. I.e. you're > forced to create a cleanup with a named function -- right? Thanks for taking the time to provide this feedback. I think what you are saying aligns with Tom's final position, that passing a function pointer or lambda to the cleanup_function constructor is better. My motivation for bringing this patch set up at all was to address Tom's comment in this email: https://www.sourceware.org/ml/gdb-patches/2019-01/msg00156.html Where he said: I looked at replacing scoped_finish_thread_state, but that one seemed reasonable to keep as is, to me, because it is used in several spots, and I didn't want to repeat the similar lambdas everywhere. By passing the function pointer into the type creation I had hoped to address this concern. Maybe there's some other reason why scoped_finish_thread_state is different, in which case I apologise for wasting everyone's time, but right now it appears to me that scoped_finish_thread_state is no different to cleanup_function, it's just used more. I think if we're going to put in a generic solution (which I think is a good thing) then we should either, make sure we understand why scoped_finish_thread_state is different (and what the rules are for when to use the generic, and when to create a class), or, make sure the generic is suitable to replace scoped_finish_thread_state. (I'm not trying to pick on scoped_finish_thread_state, it was just the first example I found when originally replying to Tom.) Thanks, Andrew > > If it's the lambda itself you don't like, I think it should be > possible to add make cleanup_function's ctor have a std::bind-like interface [1], > so that you'd pass cleanup_function's ctor the function and arguments: > > template > cleanup_function (F &&func, Args &&...args); > > so you'd create the cleanup like: > > cleanup_function cleanup (delete_longjmp_breakpoint, thread); > > Since Tromey's cleanup_function is not a template, to implement such > a ctor, it would have to rely on std::function (or something like it) for > type erasure, which might heap allocate if the resulting callable is large > enough. The advantage would be that with that you can create a cleanup_function > without passing specifying the called function's type. The disadvantage is > the cost of the std::function type erasure / potential heap allocation, of course. > > But we could avoid the cost/heap if we make cleanup_function a template, > as Andrew's version is, but I have to say that I don't really like that > version's way of declaring the cleanup_function typedef: > > +/* Cleanup class that calls delete_longjmp_breakpoint. */ > +#ifdef __cpp_template_auto > +using longjmp_breakpoint_cleanup > + = cleanup_function ; > +#else > +using longjmp_breakpoint_cleanup > += cleanup_function ; > +#endif > > I think it should be possible to code cleanup_function's template > such that you could instantiate it like: > > cleanup_function > > similarly to gdb::function_view? > > That doesn't tie cleanup_function to the actual function called, just > its type, but I wouldn't see that as a disadvantage, given this way this > works with all sorts of callables, including lambdas. > > Now, ctors don't deduce template parameter types until C++17, so with > that template interface and C++11 we wouldn't be able to just write: > > cleanup_function cleanup (delete_longjmp_breakpoint, 0); > > But, that is fixable with a helper make_cleanup_function, which would > have us write: > > auto cleanup = make_cleanup_function (delete_longjmp_breakpoint, 0); > > For the optional cleanup case, we'd need to somehow spell out the > type, no way around it, but that's not too horrible with that interface, > IMO: > cleanup_function cleanup; > or: > cleanup_function cleanup; > > (and of course a typedef could put the function's type away from view) > > I'm not really sure we need a std::bind-like interface though. I'd > be super fine with the implementation simplicity of having to pass a > lambda, like in scope_exit: > > auto cleanup > = make_scope_exit ([] { delete_longjmp_breakpoint (0); }); > ... > cleanup.release (); > > It's simpler to implement, because then all you need for the > template parameter type is a generic callable: > > template > class scope_exit > ... > > Note that with Alexandrescu's scope_exit (see > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf), if you > don't need to cancel the cleanup, you can write: > > SCOPE_EXIT { delete_longjmp_breakpoint (0); }; > > which arguably looks cleaner. Some people prefer avoiding macros > that "extend" the C++ language like that, though, I think. > > [1] https://en.cppreference.com/w/cpp/utility/functional/bind > > Thanks, > Pedro Alves