From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10657 invoked by alias); 4 Jul 2008 06:05:27 -0000 Received: (qmail 10649 invoked by uid 22791); 4 Jul 2008 06:05:26 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.191) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 04 Jul 2008 06:04:57 +0000 Received: by ti-out-0910.google.com with SMTP id d10so52939tib.12 for ; Thu, 03 Jul 2008 23:04:54 -0700 (PDT) Received: by 10.110.109.12 with SMTP id h12mr26613tic.34.1215151493954; Thu, 03 Jul 2008 23:04:53 -0700 (PDT) Received: by 10.110.109.4 with HTTP; Thu, 3 Jul 2008 23:04:53 -0700 (PDT) Message-ID: Date: Fri, 04 Jul 2008 06:05:00 -0000 From: teawater To: "Pedro Alves" Subject: Re: GDB record patch 0.1.6 for GDB branch msnyder-reverse-20080609-branch release Cc: gdb-patches@sourceware.org In-Reply-To: <200807031442.41033.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <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/msg00050.txt.bz2 Hi Pedro, Thanks for your advice. I will try my best on format in next version. BTW, I use "indent -gnu xxx.c" to deal with format case. How do you think about it? Thanks, teawater On Thu, Jul 3, 2008 at 21:42, Pedro Alves wrote: > 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 >