From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61626 invoked by alias); 15 Nov 2016 14:32:17 -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 61595 invoked by uid 89); 15 Nov 2016 14:32:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:1574, transfer, Transfer 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 Nov 2016 14:32:14 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 B7FF181222; Tue, 15 Nov 2016 14:32:13 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAFEWCIU019636; Tue, 15 Nov 2016 09:32:13 -0500 Subject: Re: [RFA 01/14] Introduce py-ref.h To: Tom Tromey References: <1478497656-11832-1-git-send-email-tom@tromey.com> <1478497656-11832-2-git-send-email-tom@tromey.com> <7df8b22f-cd05-c0d6-5ede-e9dc29bca6e2@redhat.com> <8760nsiozp.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Tue, 15 Nov 2016 14:32:00 -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: <8760nsiozp.fsf@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg00364.txt.bz2 Hi Tom, On 11/12/2016 05:31 PM, Tom Tromey wrote: > + > + /* Transfer ownership from OTHER. */ > + gdbpy_ref &operator= (gdbpy_ref &&other) > + { > + /* Note that this handles self-assignment properly. */ > + std::swap (m_obj, other.m_obj); Since there's no need to worry about noexcept garantees here (destructing a gdbpy_ref can't throw), I'd rather that move would not be implemented as a swap, as that extends the lifetime of m_obj in OTHER. I.e., bugs like these will go unnoticed longer: gdbpy_ref bar = make_whatever (....); gdbpy_ref foo = make_whatever (....); if (condition) { bar = std::move (foo); some_function (foo.get ()); // whoops, passed bar.get () instead of NULL and crashing early. } I'd be happy with the idiom of implementing move-in-term-of-swap by first move-constructing, and then swapping that temporary: gdbpy_ref &operator= (gdbpy_ref &&other) { gdbpy_ref (std::move (other)).swap (*this); return *this; } Or perhaps simpler, go with something like what you had in the first version: + /* Transfer ownership from another instance. */ + gdbpy_reference &operator= (gdbpy_reference &&other) + { + Py_XDECREF (m_obj); + m_obj = other.m_obj; + other.m_obj = NULL; + return *this; + } + But add a "if (this != &other)" check. I'd tweak it to be in terms of reset() while at it: /* Transfer ownership from another instance. */ gdbpy_reference &operator= (gdbpy_reference &&other) { if (this != &other) { reset (other.m_obj); other.m_obj = NULL; } return *this; } Otherwise LGTM. Thanks, Pedro Alves