From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126454 invoked by alias); 15 Jan 2019 23:43:13 -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 126440 invoked by uid 89); 15 Jan 2019 23:43:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=canceled 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; Tue, 15 Jan 2019 23:43:10 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8D1E5D4D94; Tue, 15 Jan 2019 23:43:09 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 581051042A62; Tue, 15 Jan 2019 23:43:08 +0000 (UTC) Subject: Re: [PATCH 00/12] remove some cleanups using a cleanup function To: Tom Tromey , Andrew Burgess References: <20190109033426.16062-1-tom@tromey.com> <961d9501-23e6-9adb-a11b-da41702c4fa0@redhat.com> <20190115094157.GP3456@embecosm.com> <87ef9dttfl.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <1014fa7f-bbce-cbea-f54f-480373299809@redhat.com> Date: Tue, 15 Jan 2019 23:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <87ef9dttfl.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-01/txt/msg00366.txt.bz2 On 01/15/2019 11:03 PM, Tom Tromey wrote: >>>>>> "Andrew" == Andrew Burgess writes: > > Andrew> Maybe there's some other reason why scoped_finish_thread_state is > Andrew> different, in which case I apologise for wasting everyone's time, but > Andrew> right now it appears to me that scoped_finish_thread_state is no > Andrew> different to cleanup_function, it's just used more. > > FWIW I don't think it's a waste of time at all. There's no particular > rush for these patches and I think it's more valuable for us to agree on > what we'd like the result to look like than it is to land them quickly. Definitely agreed. Not a waste at all! I've been playing with this today, and I have a different implementation of Andrew's class that allows writing: using delete_longjmp_breakpoint_cleanup = forward_cleanup_function; Or, with a macro to eliminate redundancy: using delete_longjmp_breakpoint_cleanup = FORWARD_CLEANUP_FUNCTION (delete_longjmp_breakpoint); Naming up in the air, I just picked that as straw man. > > Andrew> I think if we're going to put in a generic solution (which I think is > Andrew> a good thing) then we should either, make sure we understand why > Andrew> scoped_finish_thread_state is different (and what the rules are for > Andrew> when to use the generic, and when to create a class), or, make sure > Andrew> the generic is suitable to replace scoped_finish_thread_state. > > Andrew> (I'm not trying to pick on scoped_finish_thread_state, it was just the > Andrew> first example I found when originally replying to Tom.) > > Maybe I was just making too big a deal out of it, but my thinking was > that writing the finish_thread_state call at each spot would be bad, > since it would be multiple copies of the same thing. But, maybe it is > actually no big deal? > > Using a template, as Pedro suggested, would remove some of the ugliness > from the series. Stuff like this: > > + auto do_invalidate > + = [=] () > + { > + this->invalidate (regnum); > + }; > + cleanup_function invalidator (do_invalidate); > > Could instead just be: > > SCOPE_EXIT { this->invalidate (regnum); } > > ... assuming we like SCOPE_EXIT (to me it seems reasonable enough). > > Anyway, I tend to think we should simply copy the scope_exit paper. If > it's accepted into C++ then someday we can just remove the gdb variant. > > Let me know if you agree; if so I can implement this. > I've also played with the template idea, basically implemented scope_exit / make_scope_exit. Seems to work nicely. I hadn't done the SCOPE_EXIT macro though, not sure it is worth it to have yet another way to write these things (thinking about newcomers' cognitive load, having to learn all the different things) -- of all the make_scope_exit calls I have, most either take the form: auto cleanup = make_scope_exit (function); i.e., are passed a function pointer, can do without a lambda, and/or need access to the scope_exit object to cancel it. But I can give it a try for sure. It may be clearer code to standardize writing: auto cleanup = make_scope_exit (function); cleanup.release (); when the cleanup may need to be canceled and SCOPE_EXIT { function (); } when it doesn't? I won't be able to finish this today (I'd like to clean up a couple hacks here and there), but I'll post something tomorrow so we can all see and decide a way forward. Thanks, Pedro Alves