From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6932 invoked by alias); 3 Jan 2010 05:32:54 -0000 Received: (qmail 6920 invoked by uid 22791); 3 Jan 2010 05:32:52 -0000 X-SWARE-Spam-Status: No, hits=-2.4 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; Sun, 03 Jan 2010 05:32:49 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 484F52BAB7D; Sun, 3 Jan 2010 00:32:47 -0500 (EST) 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 V7MVEitgUjWv; Sun, 3 Jan 2010 00:32:47 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A8BD62BAB5A; Sun, 3 Jan 2010 00:32:46 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 14D1DF5937; Sun, 3 Jan 2010 06:32:13 +0100 (CET) Date: Sun, 03 Jan 2010 05:32:00 -0000 From: Joel Brobecker To: Michael Cc: gdb-patches@sourceware.org Subject: Re: [RFA/i387] improve the output of 'info float' (was: "Re: gdb-patch mailing list") Message-ID: <20100103053213.GA2788@adacore.com> References: <4B3E4DC6.7020901@cyberfiber.org> <20100102043704.GR548@adacore.com> <4B3F03C4.1040104@cyberfiber.org> <4B3F083C.2080300@cyberfiber.org> <20100102093213.GX2788@adacore.com> <20100102093302.GA12123@adacore.com> <4B3F252A.30504@cyberfiber.org> <20100102111708.GV548@adacore.com> <4B3F3442.60900@cyberfiber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B3F3442.60900@cyberfiber.org> 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-01/txt/msg00036.txt.bz2 Michael, I changed the subject to something meaningful; you have a *much* higher chance of attracting the eye of the appropriate maintainer if you use descriptive subjects. > I suppose that indeed something went wrong, don't know what exactly, > here's the new patch :) Much better indeed! Glad to see that things are getting in order. I would have liked to have a more detailed description of *what* you are trying to achieve, here, and also *why*. The changes you are proposing on some of the code are very small and the size of the blocks you are editing is large enough to make spotting differences harder. <> is just not informative at all, and ... > +2010-01-02 Michael Baars > + > + * i387-tdep.c: Maintenance update on 'info float' ... not acceptable as a ChangeLog entry. 1. You need to specify which functions are impacted; 2. You need to provide a more descriptive text. For instance: Improve the formatting of the "info float" command, or some such. Even that, considering your change, seems too vague. Have a look at gdb/ChangeLog-2009 to get a feel of how others have written their ChangeLog entries. The following is only going to be a unofficial review, because I will defer to the judgment of our x86/x86_64 maintainer. But he's fairly busy, so I will help you cleanup the patch before-hand. MarkK, feel free to take over at any time! > -/* Print the status word STATUS. */ > - > +/* Print the status word. */ This part, I am afraid, is not acceptable. You version of the description is less informative than the initial one. FYI, there is a convention at least in GDB to spell in the function documentation the name of the parameters in all caps... So, to a GDB developer, the initial function description makes perfect sense. > + fprintf_filtered (file, "status word : %s\n", > + hex_string_custom(status, 4)); The formatting is incorrect. hex_string_custom should be indented to match the indentation one column right of the '(' above: fprintf_filtered (file, "status word : %s\n", hex_string_custom(status, 4)); (make sure to use tabs where you would otherwise use 8 spaces) > + /* Precision */ > + fprintf_filtered (file, "%s ", (status & 0x0020) ? "PE" : " "); > + /* Underflow */ > + fprintf_filtered (file, "%s ", (status & 0x0010) ? "UE" : " "); > + /* Overflow */ > + fprintf_filtered (file, "%s ", (status & 0x0008) ? "OE" : " "); > + /* Zero Devide */ > + fprintf_filtered (file, "%s ", (status & 0x0004) ? "ZE" : " "); > + /* Denormalized operand */ > + fprintf_filtered (file, "%s ", (status & 0x0002) ? "DE" : " "); > + /* Invalid operation */ > + fprintf_filtered (file, "%s ", (status & 0x0001) ? "IE" : " "); I personally think that the added comments are overkill, but I'm otherwise OK with them. Mark might have a different opinion on this. I have a question, though, why did you reverse the order of the flags being printed? I supect because of endianness? > + fprintf_filtered (file, " stack fault : %s\n", > + (status & 0x0040) ? "SF" : " "); > + fprintf_filtered (file, " error summary status : %s\n", > + (status & 0x0080) ? "ES" : " "); > + > + fprintf_filtered (file, " condition code : "); > + > + fprintf_filtered (file, "%s ", (status & 0x4000) ? "C3" : " "); > + fprintf_filtered (file, "%s ", (status & 0x0400) ? "C2" : " "); > + fprintf_filtered (file, "%s ", (status & 0x0200) ? "C1" : " "); > + fprintf_filtered (file, "%s ", (status & 0x0100) ? "C0" : " "); > + fprintf_filtered (file, "\n"); Personally, I liked the original output format better. Mark? > -/* Print the control word CONTROL. */ > - > +/* Print the control word. */ Same as above - let's just keep the original function description. > static void > print_i387_control_word (unsigned int control, struct ui_file *file) The general comments I made above also apply to this function. I will let Mark tell you whether he likes your change of output or not. Personally, I liked the previous output better - much more compact and easier to read, there are too many decorations in your proposed output and it's harder to isolate the relevant information. This reminds me of the studies made by Airbus and Boeing about intrument readability. Boeing chose to keep the needle approach in various indicators because pilots can tell at a glance that a needle is roughly at its intended position, whereas it's impossible to do when the indicator is a plain number. You have to read the number and interpret it before you can tell. -- Joel