From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2032 invoked by alias); 15 Mar 2010 01:00:58 -0000 Received: (qmail 2024 invoked by uid 22791); 15 Mar 2010 01:00:57 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 15 Mar 2010 01:00:53 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 321BD2BAB73; Sun, 14 Mar 2010 21:00:52 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id YqXW+2guoqFX; Sun, 14 Mar 2010 21:00:52 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CE5E12BAB6D; Sun, 14 Mar 2010 21:00:51 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C97FFF5917; Sun, 14 Mar 2010 18:00:47 -0700 (PDT) Date: Mon, 15 Mar 2010 01:00:00 -0000 From: Joel Brobecker To: Holger Hans Peter Freyther , teawater@gmail.com Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Empty if body in linux-record.c Message-ID: <20100315010047.GG3045@adacore.com> References: <201003141348.24756.holger@freyther.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201003141348.24756.holger@freyther.de> User-Agent: Mutt/1.5.20 (2009-06-14) 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: 2010-03/txt/msg00519.txt.bz2 > 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