From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26556 invoked by alias); 4 Oct 2011 16:37:02 -0000 Received: (qmail 26187 invoked by uid 22791); 4 Oct 2011 16:36:59 -0000 X-SWARE-Spam-Status: No, hits=-7.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Tue, 04 Oct 2011 16:36:43 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p94GaeFV022140 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 4 Oct 2011 12:36:41 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p94GaeHj023000; Tue, 4 Oct 2011 12:36:40 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p94GadTJ024180; Tue, 4 Oct 2011 12:36:39 -0400 From: Tom Tromey To: Li Yu Cc: gdb-patches@sourceware.org, Paul Koning Subject: Re: [PATCH v2] gdb/python: add missing handling for anonymous members of struct and union References: <4E8595F6.7080004@gmail.com> Date: Tue, 04 Oct 2011 16:37:00 -0000 In-Reply-To: <4E8595F6.7080004@gmail.com> (Li Yu's message of "Fri, 30 Sep 2011 18:12:06 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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-10/txt/msg00097.txt.bz2 >>>>> "Li" == Li Yu writes: Li> gdb.Type.fields() missed handling for anonymous members. Li> This patch fix it, below are details: Thanks. Do you have a copyright assignment in place? If not, you will need one. Contact me off-list and I will get you started. Li> Assume that we have a c source as below: This patch needs a test case; so I suggest converting this into the right form for the test suite. Li> +struct __typy_iterator_object Don't use leading underscores for the name here. Li> + typy_iterator_object *iter_obj = (typy_iterator_object *) self, *child_iter_obj; This line is too long. Just make a second declaration for child_iter_obj. Li> + char *name; Probably could be const. Li> + while (iter_obj->child) /* deepest anonymous member first */ Li> + { The braces are in the wrong place for the GNU style. They should be in 2 more spaces; see the GNU coding standards for details. This occurs in a couple of places. Li> +restart: Li> + while (iter_obj->field >= TYPE_NFIELDS (type)) Li> + { Li> + iter_obj = iter_obj->parent; Li> + if (!iter_obj) Li> + return NULL; This seems strange. This returns NULL without setting a Python exception. Also the reference counting seems odd. I guess I don't understand why the typy_iterator_object has references to other iterator objects, instead of plain gdb state. References to other objects means that you at least need to decref them when the iterator object is destroyed. Li> + Py_DECREF(iter_obj->child); Missing space before the paren. This occurs in a few spots. Li> + name = TYPE_FIELD_NAME (type, iter_obj->field); Li> + if (!name) Li> + goto abort_clean; Why is this here? I think you would have to check to be sure that there is never a NULL field name created by gdb; in which case this should be some kind of internal error. Li> + if (name[0]) /* mostly cases */ I don't understand this comment. I suggest just deleting it. Li> + /* handing for anonymous members here */ Comments in gdb should be full sentences: start with an upper-case letter, end with a period followed by two spaces. See GNU standards. Li> + goto abort_clean;; Double semicolon. Li> + child_iter_obj = (typy_iterator_object*)typy_make_iter (child_pytype, iter_obj->kind); Line too long, should be wrapped. Also a missing space after the first ")". I don't understand why the iterator iterates into sub-objects. Why not just have a flat iterator? That is, return a field with no name whose type is some structure, and then let the caller iterate over that type if need be. Tom