From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34468 invoked by alias); 11 Nov 2016 04:03:19 -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 34381 invoked by uid 89); 11 Nov 2016 04:03:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=gross, lvalue, claims, our 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; Fri, 11 Nov 2016 04:03:14 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (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 957A1C04D2F5; Fri, 11 Nov 2016 04:03:13 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAB43CjK010878; Thu, 10 Nov 2016 23:03:12 -0500 Subject: Re: [RFA 00/14] add a smart pointer for PyObject* To: Tom Tromey References: <1478497656-11832-1-git-send-email-tom@tromey.com> <7f51de8d-912c-01d9-815f-85cc3c351f0c@redhat.com> <87wpgaitam.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Fri, 11 Nov 2016 04:03:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <87wpgaitam.fsf@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg00285.txt.bz2 On 11/11/2016 03:34 AM, Tom Tromey wrote: > Pedro> gdbpy_reference sort_func = module.GetAttrString ("execute_frame_filters"); > > Pedro> instead of: > > Pedro> gdbpy_reference sort_func (PyObject_GetAttrString (module.get (), > Pedro> "execute_frame_filters")); > > Pedro> etc. > > This particular one I chose not to do, on the theory that assignment > from another gdbpy_reference does a Py_INCREF. That is, this addition > would mean that one assignment operator claims ownership of an existing > reference, while another assignment operator would create a new > reference. I think that theory is incorrect. If module.GetAttrString() returns a gdbpy_reference, then RVO kicks in and no copy is made at all. If RVO doesn't kick in, because the compiler fails to optimize, then it's the move constructor that is called, since the return of module.GetAttrString() is an rvalue in this context. Likewise, when assigning to an existing variable: gdbpy_reference sort_func; ... sort_func = module.GetAttrString ("execute_frame_filters"); then it's the move assignment operator that is called. Thus there should be no increfs here at all. No chance of introducing bugs. I had that in mind here too, but didn't spell it out, sorry about that. This is like returning std::unique_ptr from functions (including C++14 std::make_unique): extern std::unique_ptr<...> make_something (); std::unique_ptr<...> foo = make_something (); The assignment only works because that's actually a move. Returning std::unique_ptr makes sure that you don't try to assign the result of the function call to a raw pointer directly. The copy assignment operator is only called when you copy from an lvalue, like e.g.: gdbpy_reference ref1, ref2; ... ref1 = ref2; And in this case clearly you do want to incref. There's another safety advantage to having functions return gdbpy_references directly. Consider usages like: if (module.GetAttrString ("execute_frame_filters") == NULL) (I made that up, I assume there are cases like this, but I didn't check.) If GetAttrString returns a PyObject *, then this leaks. If instead it returns a gdbpy_reference, it does not. > Anyway, I didn't do this just because it requires more thought; and > there were a lot of patches already; and I figured that it could be done > later if desirable. I hope I didn't came across as suggesting you'd do this now. > > Pedro> Also, somewhat related, I briefly looked at making our custom Python > Pedro> types C++ classes before, so they could themselves hold other > Pedro> C++ classes, std::string, etc., though I didn't find how to > Pedro> coerce Python to use operator new with PyTypeObject types. > Pedro> There must be some way though. > > I haven't looked at all into prior art for C++ Python APIs; which might > be worth doing. One "easy" way to do it would be to store a pointer to > a C++ object which would be "delete"d in the Python object-destruction > callback (gross due to extra allocation of course). Another possible > way would be placement new. But maybe there's something even nicer. Yeah, definitely worth looking at what other APIs are doing. Thanks, Pedro Alves