From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13845 invoked by alias); 3 Jul 2008 13:43:54 -0000 Received: (qmail 13808 invoked by uid 22791); 3 Jul 2008 13:43:49 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 03 Jul 2008 13:42:40 +0000 Received: (qmail 30463 invoked from network); 3 Jul 2008 13:42:38 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 3 Jul 2008 13:42:38 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: GDB record patch 0.1.6 for GDB branch msnyder-reverse-20080609-branch release Date: Thu, 03 Jul 2008 13:43:00 -0000 User-Agent: KMail/1.9.9 Cc: teawater References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200807031442.41033.pedro@codesourcery.com> 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-07/txt/msg00034.txt.bz2 Hi, A Thursday 03 July 2008 13:58:33, teawater wrote: > On Thu, Jul 3, 2008 at 17:29, teawater wrote: > > Please give me your advice about GDB record. Thanks a lot. :) I have one request to make. I tried to skim through your patch, looking at places that touch common code, but the fact that a lot of it seems to be mere formatting changes, makes it very hard to see what's going on. Could you make sure your next revision doesn't have those spurious formatting changes? E.g., infrun.c has a lot of those. The benefit to you, is that if you get rid of those changes, it will be easier to maintain your patch when the base your applying it to changes, that is, you'll get lesser conflicts. I'd advise you to look a bit at these: http://sourceware.org/gdb/current/onlinedocs/gdbint_15.html#SEC120 http://www.gnu.org/prep/standards/standards.html#Formatting Pure formatting fixes should be submitted/done separatelly. It's usually ok to fix the formatting of surrounding code where you're making fixes or adding functionality, but spurious changes all over makes it distracting and hard to make sense of the patch. There are many formatting issues you're making in your new code, that sooner or later you'll have to fix. Better be aware of it early on. E.g.: + if (record_arch_list_add_reg (I386_EAX_REGNUM)) + { + return (-1); + } Should be: + if (record_arch_list_add_reg (I386_EAX_REGNUM)) + return -1; Doesn't mean that you need to rush and fix them all now (if you do, great), but those will have to be fixed anyway, if/when inclusion is considered. Thanks for your hard work! -- Pedro Alves