Re: GDB record patch 0.1.6 for GDB branch msnyder-reverse-20080609-branch release

[Pedro Alves <pedro@codesourcery.com>]

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