From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29746 invoked by alias); 21 Feb 2008 22:11:56 -0000 Received: (qmail 29736 invoked by uid 22791); 21 Feb 2008 22:11:55 -0000 X-Spam-Check-By: sourceware.org Received: from bluesmobile.specifix.com (HELO bluesmobile.specifix.com) (216.129.118.140) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 21 Feb 2008 22:11:38 +0000 Received: from [127.0.0.1] (bluesmobile.specifix.com [216.129.118.140]) by bluesmobile.specifix.com (Postfix) with ESMTP id 0A8B63BD2E; Thu, 21 Feb 2008 14:11:35 -0800 (PST) Subject: Re: simlpe patch implements eval command (with printf-like format and args) From: Michael Snyder To: Yakov Lerner Cc: gdb-patches@sourceware.org In-Reply-To: References: Content-Type: text/plain Date: Thu, 21 Feb 2008 22:37:00 -0000 Message-Id: <1203631895.19253.213.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.10.3 (2.10.3-7.fc7) Content-Transfer-Encoding: 7bit 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: 2008-02/txt/msg00352.txt.bz2 On Thu, 2008-02-21 at 11:20 +0200, Yakov Lerner wrote: > This simple patch implements eval command with printf-like syntax: > > eval "printf-like-format", comma-separated args > > The patch is against cvs-checkedout source. Suggestions are welcome. > Implementation is very simple. Not bad at all, for a first contribution. But I suggest you try to limit the scope a little bit and make it still simpler -- maybe have it accept only strings, floats and ints to start -- and see if you can't implement it less intrusively eg. without taking over the function for another command and introducing new cross-module infrastructure. Then we can maybe iteratively improve on it. I'll give you some style and structure feedback on this one: > > +/* Grow allocated buffer and appens to it, without causing quadratic > slowdown */ "append", of course -- and comments must be punctuated (in this case, period followed by two spaces". Plus indentation -- consult the GNU coding standard. > +/* Grow allocated buffer and appens to it, without causing quadratic > slowdown */ As above. > +static void > +eval_command (char *arg, int from_tty) > +{ > + struct cleanup *old_cleanups; > + char *str = gdb_own_xasprintf (arg); > + > + old_cleanups = make_cleanup (free_current_contents, &str); > + execute_command (str, from_tty); > + do_cleanups (old_chain); > +} old_chain is undefined. Probably should be "old_cleanups"? Actually, except for the comment tails, you've done a commendable job of following the coding standard. I'd like to see an implementation that's limited enough not to require the "append" function. Thanks, Michael