From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4989 invoked by alias); 3 Jul 2011 16:18:43 -0000 Received: (qmail 4981 invoked by uid 22791); 3 Jul 2011 16:18:43 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 03 Jul 2011 16:18:25 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p63GINrC031363 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 3 Jul 2011 12:18:23 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p63GILHF024031; Sun, 3 Jul 2011 12:18:22 -0400 From: Phil Muldoon To: Matt Rice Cc: gdb-patches@sourceware.org Subject: Re: [patch] [python] find_line_pc_range References: Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Sun, 03 Jul 2011 16:35:00 -0000 In-Reply-To: (Matt Rice's message of "Fri, 1 Jul 2011 21:02:42 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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 X-SW-Source: 2011-07/txt/msg00075.txt.bz2 Matt Rice writes: > don't know a whole much about python but, > would it be better to return None on error, instead of a tuple containing Nones? Thanks. I would return either an exception or None depending on the situational context of the failure. If a None type scenario is expected, and normal then it is okay to return that, given that the Python scripter would understand the reason for that. So documentation is key here. I think find_line_pc_range is just a utility function, which can very well not find the line range. So I would replicate that functionality in your API, except I would return a single Py_None over a tuple. > +salpy_find_line_pc_range (PyObject *self, PyObject *args) > +{ > + struct symtab_and_line *sal = NULL; > + CORE_ADDR start_pc, end_pc; > + PyObject *start; > + PyObject *end; > + PyObject *ret_tuple; > + > + SALPY_REQUIRE_VALID (self, sal); > + > + ret_tuple = PyTuple_New (2); This call can fail and return NULL, so in this case you need to return NULL to propagate the failure. > + if (find_line_pc_range (*sal, &start_pc, &end_pc)) > + { > + > + start = gdb_py_long_from_longest (start_pc); > + end = gdb_py_long_from_longest (end_pc); > + PyTuple_SET_ITEM (ret_tuple, 0, start); > + PyTuple_SET_ITEM (ret_tuple, 1, end); > + } start and end could be local to this block I think. Also gdb_py_long_from_longest is macro that points to Py Long_FromUnsignedLongLong. This can also return NULL which indicates a failure, so similar to above, you need to check the return and return NULL if one of them fails. If one of them does return NULL, you also need to appropriately clean-up previous references that were created in Python. We normally do this by creating a local error: goto. A minor nit, SET_ITEM does not do any error checking, while SetItem does. As this is a new tuple, then it is ok to use it. But normally (and personally, in new tuples) I always use the error checking equivalent. + else + { + PyTuple_SET_ITEM (ret_tuple, 0, Py_None); + PyTuple_SET_ITEM (ret_tuple, 1, Py_None); + } + In this case I would just return Py_None (see first paragraph). It seems a little redundant to return a tuple with Py_None for both elements, and this will always be the case on a lookup failure with find_line_pc_range. Make sure you incref Py_None before returning it, also. Also, this patch needs tests. Thanks for the patch and your work! Cheers, Phil