From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13080 invoked by alias); 16 Mar 2010 08:01:32 -0000 Received: (qmail 12928 invoked by uid 22791); 16 Mar 2010 08:01:30 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40 X-Spam-Check-By: sourceware.org Received: from mail-pw0-f41.google.com (HELO mail-pw0-f41.google.com) (209.85.160.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 16 Mar 2010 08:01:25 +0000 Received: by pwi4 with SMTP id 4so1080452pwi.0 for ; Tue, 16 Mar 2010 01:01:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.142.67.34 with SMTP id p34mr432356wfa.335.1268726484151; Tue, 16 Mar 2010 01:01:24 -0700 (PDT) In-Reply-To: <20100315010047.GG3045@adacore.com> References: <201003141348.24756.holger@freyther.de> <20100315010047.GG3045@adacore.com> From: Hui Zhu Date: Tue, 16 Mar 2010 08:01:00 -0000 Message-ID: Subject: Re: [PATCH] Empty if body in linux-record.c To: Joel Brobecker , Holger Hans Peter Freyther Cc: gdb-patches@sourceware.org, Michael Snyder Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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/msg00574.txt.bz2 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 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: > > =A01. You should indicate how you tested this. Normally, you run > =A0 =A0 the testsuite before and after, and compare the associated gdb.sum > =A0 =A0 file, to make sure that you did not introduce any regression. > > =A0 =A0 In this particular case, you also need to make sure that process > =A0 =A0 record testing is activated, which it is not by default. > =A0 =A0 Please have a look at > =A0 =A0 http://sourceware.org/gdb/wiki/ProcessRecord#Testing. > > =A0 2. Your ChangeLog entry is much too verbose: > >> =A0 =A0 =A0 * linux-record.c (record_linux_msghdr): Fix a compiler warni= ng >> =A0 =A0 =A0 about an empty if body that has existed since the addition o= f the >> =A0 =A0 =A0 code. This change will change the return value of the functi= on from >> =A0 =A0 =A0 always returning -1 to returning -1 only if calling record_a= rch_list_add_mem >> =A0 =A0 =A0 is failing. The inner for loop will also be iterated more th= an once >> =A0 =A0 =A0 now. > > =A0 =A0 =A0You just need to say *what* you've done, not why or how. > =A0 =A0 =A0This is explained in more details in the Gnu Coding Standards > =A0 =A0 =A0manual. (I sometimes indulge myself, and add an explaination, > =A0 =A0 =A0but it's because it's a very very short one) > > =A0 =A0 =A0On possibility would be: > > =A0 =A0 =A0* linux-record.c (record_linux_msghdr): Remove unintended semi= colons. > > Would you like write access to the GDB repository? > > -- > Joel >