From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12090 invoked by alias); 22 Dec 2011 11:33:39 -0000 Received: (qmail 12075 invoked by uid 22791); 22 Dec 2011 11:33:37 -0000 X-SWARE-Spam-Status: No, hits=-3.0 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-qw0-f41.google.com (HELO mail-qw0-f41.google.com) (209.85.216.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Dec 2011 11:33:23 +0000 Received: by qabg40 with SMTP id g40so3000267qab.0 for ; Thu, 22 Dec 2011 03:33:23 -0800 (PST) Received: by 10.224.10.14 with SMTP id n14mr12868582qan.64.1324553603083; Thu, 22 Dec 2011 03:33:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.224.10.14 with SMTP id n14mr12868572qan.64.1324553603007; Thu, 22 Dec 2011 03:33:23 -0800 (PST) Received: by 10.224.72.210 with HTTP; Thu, 22 Dec 2011 03:33:22 -0800 (PST) In-Reply-To: <20111222051725.GO23376@adacore.com> References: <20111222051725.GO23376@adacore.com> Date: Thu, 22 Dec 2011 11:48:00 -0000 Message-ID: Subject: Re: [RFC] A new command 'explore' From: Siva Chandra To: Joel Brobecker Cc: gdb-patches@sourceware.org X-System-Of-Record: true Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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-12/txt/msg00781.txt.bz2 Thanks for taking a look Joel. My comments inline. Siva > +import gdb Joel > GDB already pre-imports module `gdb', and it's documented in the GDB Joel > manual. So I think you can get rid of this import. Though, I wonder Joel > if that's the recommended style or not. Tom? This is the convention(?) in other files in gdb/python/lib/gdb/command. Joel > It seems that the design is centered around these various *Explorer Joel > classes implementing a defined interface. =A0If that's the case, Joel > I think it would make it clearer if they inherited from an abstract Joel > class where you can then clearly describe what the interface is, Joel > and what each element is expected to do. Put an assertion in each Joel > abstract method to raise an exception if the method is not implement= ed Joel > by the deriving class. Joel > Joel > Also, almost everything is declared @staticmethod. Is that typical Joel > Python? I don't have enough experience with expert Python to really Joel > say anything, but it almost seems like you're taking the problem Joel > backwards. Let's imagine that you had one abstract Explorer class Joel > which did all the generic stuff, and then a collection of *Explorer Joel > classes which derived from it, you'd then only need a factory that Joel > the commands would invoke, and then the rest would be standard pytho= n. Joel > It would avoid all the @staticmethod, as well as the explicit class Joel > names in the method invocations. Joel > Joel > This is not a request to change, but just some suggestions based Joel > on my limited understanding of the code and of Python in the general. Joel > This code is self contained, so it does not matter as much how it Joel > is designed. Please do what you think is best. I did have inheritance in my first implementation. But soon realized that I am essentially implementing stateless functions. Hence, I removed the inheritance, kept the classes, and made the functions static (to imply that they are stateless). In fact, we need not have the classes as well. Like you have noted in your penultimate sentence above, this is really not an API for others to use. So, isn't an abstract base class and inheritance an overkill for a Python implementation here? Siva > + =A0 =A0def get_type_from_str(type_str): Siva > + =A0 =A0 =A0 =A0try: Siva > + =A0 =A0 =A0 =A0 =A0 =A0# Assume the current language to be C/C++ a= nd make a try. Siva > + =A0 =A0 =A0 =A0 =A0 =A0return gdb.parse_and_eval("(%s *)0" % type_str).type.target() Siva > + =A0 =A0 =A0 =A0except RuntimeError: Siva > + =A0 =A0 =A0 =A0 =A0 =A0# If assumption of current language to be C= /C++ was wrong, then Siva > + =A0 =A0 =A0 =A0 =A0 =A0# lookup the type using the API. Siva > + =A0 =A0 =A0 =A0 =A0 =A0try: Siva > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return gdb.lookup_type(type_str) Siva > + =A0 =A0 =A0 =A0 =A0 =A0except RuntimeError: Siva> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return None Joel > We really need to think about giving access to parameter values Joel > from python.... That would make the code above a little easier, Joel > as you'd be able to temporarily force the language to C... Like I have said in my response to Tom, I am OK to take a look at this once this patch is cleared. Thanks, Siva Chandra