From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30174 invoked by alias); 21 Oct 2009 15:23:44 -0000 Received: (qmail 30164 invoked by uid 22791); 21 Oct 2009 15:23:43 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 21 Oct 2009 15:23:36 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 7A3B13F019; Wed, 21 Oct 2009 08:23:35 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost3.vmware.com (Postfix) with ESMTP id 6FFC7CD9F9; Wed, 21 Oct 2009 08:23:35 -0700 (PDT) Message-ID: <4ADF25F4.70804@vmware.com> Date: Wed, 21 Oct 2009 15:23:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" , Hui Zhu Subject: Re: [RFA] Expand "info record" References: <4AD358E7.50009@vmware.com> <200910210122.06549.pedro@codesourcery.com> <4ADE647D.10607@vmware.com> <200910211154.06992.pedro@codesourcery.com> In-Reply-To: <200910211154.06992.pedro@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2009-10/txt/msg00497.txt.bz2 Pedro Alves wrote: > On Wednesday 21 October 2009 02:31:41, Michael Snyder wrote: >> Pedro Alves wrote: >>> On Tuesday 20 October 2009 23:25:30, Michael Snyder wrote: >>> >>>>>> + /* Display instruction number for last instruction in the log. */ >>>>>> + printf_filtered (_("Highest recorded instruction number is %llu.\n"), >>>>>> + record_insn_count ? record_insn_count - 1 : 0); >>>>> Why the conditional subtraction? >>>> Because I don't want it to say "-1". >>> Okay, that much is obvious, but how can you reach here >>> with record_insn_count == 0, given that you check if you >>> have a log at all a bit above? >> Maybe not -- but I'm a belt-and-suspenders guy. >> I don't believe in not checking for something just because >> it "can't happen". What if somebody changed the check above? > > That's why we comment non-obvious code, or add > gdb_asserts (could be overkill), or warnings. The worse that can > happen if somebody changes the check above, is the new bug is > quickly exposed because gdb starting printing '(ULONGEST) -1'. > Masking a buglet like that is wrong, IMO. > >> Yeah, you've convinced me. Thanks for the prodding, >> and please see new revision. I'll make that conditional >> go away too. ;-) > > Thanks. I could nit some more, but the all the info seems > there. ;-) I'm fine with the patch. > > I don't see "info record" mentioned in the docs. Only: > @kindex info record insn-number > @item info record insn-number > Show the current number of recorded instructions. > > Was there a corresponding documentation patch? > > FTR, it still pains me when I see this: >> + if (current_target.to_stratum == record_stratum) >> + { > > especially more so now that we have a stratum higher > than record_statum... We do? ... OK -- how would you suggest doing this sort of test? I'll get right to work on the docs patch, thanks for reminding me.