From patchwork Mon Jan 21 20:12:52 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 31141 Received: (qmail 116248 invoked by alias); 21 Jan 2019 20:12:59 -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 116229 invoked by uid 89); 21 Jan 2019 20:12:57 -0000 Authentication-Results: sourceware.org; auth=none 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, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=papers, 2014, Eg, captured 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; Mon, 21 Jan 2019 20:12:55 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8B67587622; Mon, 21 Jan 2019 20:12:54 +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 4C15B5D6AA; Mon, 21 Jan 2019 20:12:53 +0000 (UTC) Subject: Re: [PATCH 00/12] remove some cleanups using a cleanup function To: Tom Tromey References: <20190109033426.16062-1-tom@tromey.com> <961d9501-23e6-9adb-a11b-da41702c4fa0@redhat.com> <20190115094157.GP3456@embecosm.com> <87ef9dttfl.fsf@tromey.com> <1014fa7f-bbce-cbea-f54f-480373299809@redhat.com> <87ef9bm0a8.fsf@tromey.com> Cc: Andrew Burgess , gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Mon, 21 Jan 2019 20:12:52 +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: <87ef9bm0a8.fsf@tromey.com> On 01/17/2019 09:39 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > > Pedro> See patch below. Since cleanup_function turned into scope_exit, > Pedro> this new class became forward_scope_exit, because it's a "forwarding" > Pedro> version of scope_exit? I'm really not that sure about that name... > Pedro> wrap_scope_exit came to mind too. Or maybe call it cleanup_function? > > The name seems reasonable enough to me. > > Pedro> Ideally we'd find a way to implement SCOPE_FAILURE/SCOPE_SUCCESS > Pedro> in C++11 instead... > > Maybe we should start a C++17 meta-bug to collect future changes. > > Pedro> I think this is the first time I had found a use for a function-try-block, > Pedro> see scope_exit ctor... > > TIL! > > Pedro> Here is patch with everything together. WDYT? > > I think it all looks good. One nit and one question... > > Pedro> +/* A forward_scope_exit is like scope_exit, but instead of giving it a > Pedro> + callable, you instead specialize it for a given cleanup function, > Pedro> + and the generated class automatically has a constructor with the > > I think it would make sense to explain the origin of the "forward" name > somewhere in this comment. I added this locally: > > Pedro> +#define SCOPE_EXIT \ > Pedro> + auto CONCAT(scope_exit_, __LINE__) = ::detail::scope_exit_lhs () + [&] () > [...] > Pedro> + auto invalidator > Pedro> + = make_scope_exit ([&] { this->invalidate (regnum); }); > > In the current spots it doesn't matter, but I tend to think it's better > to capture by value rather than by reference. Capturing by value would mean that you wouldn't be able to refer to objects of non-copyable types, though. E.g., std::unique_ptr. Unless we took the effort to capture pointers to such objects, of course. I don't think the result would be as nice. The nice thing about capturing by reference is that it always Just Works with no syntax overhead. > The local being captured > might well be reused in the function. > Indeed, that's a valid concern. How about documenting it, something like: -/* Register a block of code to run on scope exit. */ +/* Register a block of code to run on scope exit. Note that the local + context is captured by reference, which means you should be careful + to avoid the case of locals being captured being inadvertently + reused/changed in the function before the scope exit runs. */ #define SCOPE_EXIT \ ? Maybe there's a way to simplify that sentence. > On the other hand, this would be a difference from the spec. Yeah. The SCOPE_EXIT macro itself isn't part of the p0052r5 proposal AFAICT. It's part of Alexandruscu's original SCOPE_EXIT idea though, shown on slide 20/46 (pdf page 14) of: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf IMHO diverging would cause people familiar with the idea confused. I'm working on merging my changes to each appropriate patch of your series. Thanks, Pedro Alves diff --git c/gdb/common/forward-scope-exit.h w/gdb/common/forward-scope-exit.h index 6b51e296b6..5ce230f0dc 100644 --- c/gdb/common/forward-scope-exit.h +++ w/gdb/common/forward-scope-exit.h @@ -64,7 +64,11 @@ callable template type when you create the gdb::optional: gdb:optional> -*/ + + The "forward" naming fits both purposes shown above -- the class + "forwards" ctor arguments to the wrapped cleanup function at scope + exit time, and can also be used to "forward declare" + scope_exit-like objects. */ namespace detail {