Re: [PATCH] Empty if body in linux-record.c

[Hui Zhu <teawater at gmail dot com>]

That is great!
Thanks Holger.

Hi Joel,

Do you think I can help this guy check-in this patch directly?

Thanks,
Hui

On Mon, Mar 15, 2010 at 09:00, Joel Brobecker <brobecker@adacore.com> wrote:
>> this is one the issues reported by "clang --analyze" and it appears to
>> me that the code was broken in the same way since the initial check-in
>> of these routines, looking at the tests in gdb.record it does not appear
>> that these are covered at all.
>
> I think you are right. Hui should be able to confirm that the code
> was indeed broken and that the trailing semicolon was not intended.
>
>> I'm not really known to the patch contribution of gdb so I'm not sure if I'm
>> following the proper process here...anyway here is a patch with a
>> ChangeLog entry.
>
> Pretty good for a first try :). Just a couple of comments:
>
> ?1. You should indicate how you tested this. Normally, you run
> ? ? the testsuite before and after, and compare the associated gdb.sum
> ? ? file, to make sure that you did not introduce any regression.
>
> ? ? In this particular case, you also need to make sure that process
> ? ? record testing is activated, which it is not by default.
> ? ? Please have a look at
> ? ? http://sourceware.org/gdb/wiki/ProcessRecord#Testing.
>
> ? 2. Your ChangeLog entry is much too verbose:
>
>> ? ? ? * linux-record.c (record_linux_msghdr): Fix a compiler warning
>> ? ? ? about an empty if body that has existed since the addition of the
>> ? ? ? code. This change will change the return value of the function from
>> ? ? ? always returning -1 to returning -1 only if calling record_arch_list_add_mem
>> ? ? ? is failing. The inner for loop will also be iterated more than once
>> ? ? ? now.
>
> ? ? ?You just need to say *what* you've done, not why or how.
> ? ? ?This is explained in more details in the Gnu Coding Standards
> ? ? ?manual. (I sometimes indulge myself, and add an explaination,
> ? ? ?but it's because it's a very very short one)
>
> ? ? ?On possibility would be:
>
> ? ? ?* linux-record.c (record_linux_msghdr): Remove unintended semicolons.
>
> Would you like write access to the GDB repository?
>
> --
> Joel
>