From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77511 invoked by alias); 11 Nov 2016 03:34:26 -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 77071 invoked by uid 89); 11 Nov 2016 03:34:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=gross, HTo:U*palves, claims, claim X-HELO: gproxy4-pub.mail.unifiedlayer.com Received: from gproxy4-pub.mail.unifiedlayer.com (HELO gproxy4-pub.mail.unifiedlayer.com) (69.89.23.142) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with SMTP; Fri, 11 Nov 2016 03:34:15 +0000 Received: (qmail 3802 invoked by uid 0); 11 Nov 2016 03:34:14 -0000 Received: from unknown (HELO cmgw3) (10.0.90.84) by gproxy4.mail.unifiedlayer.com with SMTP; 11 Nov 2016 03:34:14 -0000 Received: from box522.bluehost.com ([74.220.219.122]) by cmgw3 with id 6TaB1u0032f2jeq01TaEGq; Thu, 10 Nov 2016 20:34:14 -0700 X-Authority-Analysis: v=2.1 cv=WL/sABcR c=1 sm=1 tr=0 a=GsOEXm/OWkKvwdLVJsfwcA==:117 a=GsOEXm/OWkKvwdLVJsfwcA==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=L24OOQBejmoA:10 a=20KFwNOVAAAA:8 a=tmOjMkLHuL-7ZkvA9B4A:9 a=EFEiVl9YDdB7aSch:21 a=3LTD6hYtyLotdALn:21 a=e_O65bzb51kRm2y5VmPK:22 Received: from 174-16-143-211.hlrn.qwest.net ([174.16.143.211]:54394 helo=bapiya) by box522.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.86_1) (envelope-from ) id 1c52bS-0005SD-Vd; Thu, 10 Nov 2016 20:34:11 -0700 From: Tom Tromey To: Pedro Alves Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [RFA 00/14] add a smart pointer for PyObject* References: <1478497656-11832-1-git-send-email-tom@tromey.com> <7f51de8d-912c-01d9-815f-85cc3c351f0c@redhat.com> Date: Fri, 11 Nov 2016 03:34:00 -0000 In-Reply-To: <7f51de8d-912c-01d9-815f-85cc3c351f0c@redhat.com> (Pedro Alves's message of "Fri, 11 Nov 2016 01:18:39 +0000") Message-ID: <87wpgaitam.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-BWhitelist: no X-Exim-ID: 1c52bS-0005SD-Vd X-Source-Sender: 174-16-143-211.hlrn.qwest.net (bapiya) [174.16.143.211]:54394 X-Source-Auth: tom+tromey.com X-Email-Count: 6 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTIyLmJsdWVob3N0LmNvbQ== X-SW-Source: 2016-11/txt/msg00284.txt.bz2 >>>>> "Pedro" == Pedro Alves writes: Pedro> I did wonder about shortening the name to "gdbpy_ref". That's how I end Pedro> up reading "gdbpy_reference" in my mind after seeing it so many Pedro> times. :-) You already picked a shorter name as header file Pedro> name -- py-ref.h -- and that's typed way less often. :-) Pedro> Anyway, certainly not very important. Just mentioning in case Pedro> you had already considered but didn't know what others would think. I wouldn't mind. I just erred on the side of verbosity. Pedro> I wonder whether it'd be desirable to add more methods to Pedro> gdbpy_reference, so you'd have code like: 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. So, to avoid the confusion I made the PyObject* constructor explicit, and I required the use of .reset(...) to claim ownership. I'm open to basically any spelling of any of this. I just thought this approach was most likely to avoid bugs. Pedro> And maybe add some C++ classes that inherit from gdbpy_reference for Pedro> specific Python types (e.g python tuple or list), which would provide Pedro> methods that would only make sense for those types. Pedro> Did you consider things like these? Or maybe you did but thought Pedro> they'd obfuscate? I did consider subclassing and also adding some convenience methods. Other similar ideas came to mind as well, like providing wrapper functions for all the Python APIs to try to ensure ownership sanity -- basically implementing David Malcolm's reference-checker work as a bunch of templates or so. I do think this would help; while the current approach definitely improves the situation, it also leaves some bug-prone behavior. For example, PyTuple_SET_ITEM steals a reference, so calls end up looking like: PyTuple_SET_ITEM (py_arg_tuple.get (), 0, py_value_obj.release ()); ... using .get() rather than .release() would cause a bug (probably a crash). If we had a wrapper we could make calls look like: mumble_tuple_set_item (py_arg_tuple, 0, py_value_obj); ... with the first argument being unwrapped using .get() and the third with an implicit incref (or require std::move there). (An equivalent using member functions could also be done.) 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. 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. Tom