From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6741 invoked by alias); 22 Dec 2011 05:17:51 -0000 Received: (qmail 6723 invoked by uid 22791); 22 Dec 2011 05:17:48 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Dec 2011 05:17:33 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5F0D32BB3E0; Thu, 22 Dec 2011 00:17:32 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id oNqN3dCNnveF; Thu, 22 Dec 2011 00:17:32 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C263A2BB3CA; Thu, 22 Dec 2011 00:17:31 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 8435C145615; Wed, 21 Dec 2011 21:17:25 -0800 (PST) Date: Thu, 22 Dec 2011 05:21:00 -0000 From: Joel Brobecker To: Siva Chandra Cc: gdb-patches@sourceware.org Subject: Re: [RFC] A new command 'explore' Message-ID: <20111222051725.GO23376@adacore.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) 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-12/txt/msg00761.txt.bz2 Hi Siva, > 2011-12-19 Siva Chandra > > New command 'explore' which helps explore values in scope. > * data-directory/Makefile.in: Add gdb/command/explore.py > * python/lib/gdb/command/explore.py: Implemention of the 'explore' > command using the GDB Python API. > * testsuite/gdb.python/Makefile.in: Add py-explore to EXECUTABLES > * testsuite/gdb.python/py-explore.c: C program used for testing > the new 'explore' command. > * testsuite/gdb-python/py-explore.exp: Tests for the new 'explore' > command. Thanks for sending this. It's always exciting when you see people send patches using the extension language. I have a few minor comments for you... I'll try to skip the comments that Tom already made. > +import gdb GDB already pre-imports module `gdb', and it's documented in the GDB manual. So I think you can get rid of this import. Though, I wonder if that's the recommended style or not. Tom? > + _SCALAR_TYPE_LIST = [ > + gdb.TYPE_CODE_CHAR, > + gdb.TYPE_CODE_INT, > + gdb.TYPE_CODE_BOOL, > + gdb.TYPE_CODE_FLT, > + gdb.TYPE_CODE_VOID, > + gdb.TYPE_CODE_ENUM, > + ] It's not very important, but since the list is constant, make that a tuple. Generally speaking, I have noticed that people tend to use lists all the time, and many times a tuple is what they really should be using. I checked the performance delta between list and tuple a while back, and it was significant. > + @staticmethod > + def explore_expr(expr, value, is_child): I think it would be nice if each method was documented, the same way we document our code in C. > + print ("Explorer for type \'%s\' not yet available.\n" % ^^^^^^^ I don't think the backslashes are necessary here. Same elsewhere... > + def is_scalar_type(type): > + if type.code in Explorer._SCALAR_TYPE_LIST: > + return True > + else: > + return False Nitpick: return type.code in Explorer._SCALAR_TYPE_LIST ? > +class ScalarExplorer(object): > + """Internal class used to explore scalar values.""" It seems that the design is centered around these various *Explorer classes implementing a defined interface. If that's the case, I think it would make it clearer if they inherited from an abstract class where you can then clearly describe what the interface is, and what each element is expected to do. Put an assertion in each abstract method to raise an exception if the method is not implemented by the deriving class. Also, almost everything is declared @staticmethod. Is that typical Python? I don't have enough experience with expert Python to really say anything, but it almost seems like you're taking the problem backwards. Let's imagine that you had one abstract Explorer class which did all the generic stuff, and then a collection of *Explorer classes which derived from it, you'd then only need a factory that the commands would invoke, and then the rest would be standard python. It would avoid all the @staticmethod, as well as the explicit class names in the method invocations. This is not a request to change, but just some suggestions based on my limited understanding of the code and of Python in the general. This code is self contained, so it does not matter as much how it is designed. Please do what you think is best. > +class ExploreUtils(object): > + """Internal class which provides utilities for the main classes.""" > + > + @staticmethod > + def init_env(): > + Explorer.type_code_to_explorer_map = { > + gdb.TYPE_CODE_CHAR : ScalarExplorer, > + gdb.TYPE_CODE_INT : ScalarExplorer, > + gdb.TYPE_CODE_BOOL : ScalarExplorer, > + gdb.TYPE_CODE_FLT : ScalarExplorer, > + gdb.TYPE_CODE_VOID : ScalarExplorer, > + gdb.TYPE_CODE_ENUM : ScalarExplorer, > + gdb.TYPE_CODE_STRUCT : CompoundExplorer, > + gdb.TYPE_CODE_UNION : CompoundExplorer, > + gdb.TYPE_CODE_PTR : PointerExplorer, > + gdb.TYPE_CODE_TYPEDEF : TypedefExplorer, > + gdb.TYPE_CODE_ARRAY : ArrayExplorer > + } You wouldn't have to put that static map as a class variable in class Explorer. This map would be all your factory would need. I'd make that a pure function, or even a global "constant" that you'd use from your commands. > + def get_type_from_str(type_str): > + try: > + # Assume the current language to be C/C++ and make a try. > + return gdb.parse_and_eval("(%s *)0" % type_str).type.target() > + except RuntimeError: > + # If assumption of current language to be C/C++ was wrong, then > + # lookup the type using the API. > + try: > + return gdb.lookup_type(type_str) > + except RuntimeError: > + return None We really need to think about giving access to parameter values from python.... That would make the code above a little easier, as you'd be able to temporarily force the language to C... -- Joel