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

[Joel Brobecker <brobecker at adacore dot com>]

> 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