From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24904 invoked by alias); 11 Jun 2014 19:11:39 -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 24888 invoked by uid 89); 11 Jun 2014 19:11:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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; Wed, 11 Jun 2014 19:11:36 +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 s5BJBVXi029758 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 11 Jun 2014 15:11:31 -0400 Received: from barimba (ovpn-113-103.phx2.redhat.com [10.3.113.103]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s5BJBT7s014952 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Wed, 11 Jun 2014 15:11:30 -0400 From: Tom Tromey To: Alexander Smundak Cc: Eli Zaretskii , gdb-patches Subject: Re: [PATCH] Add Frame.read_register to Python API References: <83oay128ca.fsf@gnu.org> Date: Wed, 11 Jun 2014 19:11:00 -0000 In-Reply-To: (Alexander Smundak's message of "Mon, 9 Jun 2014 14:16:20 -0700") Message-ID: <87ioo7uuqm.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2014-06/txt/msg00475.txt.bz2 >>>>> "Alexander" == Alexander Smundak writes: Thanks for your patch. There's a few nits but nothing very serious. Alexander> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi Alexander> index 4688783..14564eb 100644 Alexander> --- a/gdb/doc/python.texi Alexander> +++ b/gdb/doc/python.texi Alexander> @@ -2046,9 +2046,10 @@ class InlinedFrameDecorator(FrameDecorator): Alexander> def __init__(self, fobj): Alexander> super(InlinedFrameDecorator, self).__init__(fobj) Alexander> + self.fobj = fobj Alexander> def function(self): Alexander> - frame = fobj.inferior_frame() Alexander> + frame = self.fobj.inferior_frame() Alexander> name = str(frame.name()) I think this is a nice fix but it seems unrelated to the patch at hand. Alexander> @defun Frame.find_sal () Alexander> -Return the frame's symtab and line object. Alexander> +Return the frame's @code{gdb.Symtab_and_line} object. Likewise. Alexander> +/* Implementation of gdb.Frame.read_register (self, register) -> gdb.Value. Alexander> + Returns the value of a register in this frame. */ Alexander> +static PyObject * Alexander> +frapy_read_register (PyObject *self, PyObject *args) Alexander> +{ The gdb style requests a blank line between the intro comment and the function. Alexander> + struct frame_info *frame; Alexander> + volatile struct gdb_exception except; Alexander> + int regnum = -1; Alexander> + struct value *val = NULL; Alexander> + TRY_CATCH (except, RETURN_MASK_ALL) Alexander> + { ... and also a blank line between variable declarations and code (wherever this occurs -- there's a case in the patch in a nested block). Alexander> + FRAPY_REQUIRE_VALID (self, frame); Alexander> + if (!PyArg_ParseTuple (args, "i", ®num)) Alexander> + { Alexander> + const char *regnum_str; Alexander> + PyErr_Clear(); /* Clear PyArg_ParseTuple failure above. */ Alexander> + if (PyArg_ParseTuple (args, "s", ®num_str)) Alexander> + { Alexander> + regnum = user_reg_map_name_to_regnum (get_frame_arch (frame), Alexander> + regnum_str, Alexander> + strlen (regnum_str)); Alexander> + } Alexander> + } I tend to think this would be clearer if the arguments were only parsed once and then explicit type checks were applied to the resulting object. Alexander> + return val ? value_to_value_object (val) : NULL; gdb style requests an explicit NULL check, like "return val != NULL ? ...". Alexander> +# On x86-64, PC is register 16. Alexander> +gdb_test "python print ('result = %s' % ((f0.architecture().name() != 'i386:x86-64') or f0.read_register('pc') == f0.read_register(16)))" \ Alexander> + "True" \ Alexander> + "test Frame.read_register(regnum)" A test that is arch-specific needs to be conditionalized somehow. Tom