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

[teawater <teawater at gmail dot com>]

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 <pedro@codesourcery.com> 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
>