From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9739 invoked by alias); 28 Dec 2010 05:47:53 -0000 Received: (qmail 9725 invoked by uid 22791); 28 Dec 2010 05:47:51 -0000 X-SWARE-Spam-Status: No, hits=-2.0 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; Tue, 28 Dec 2010 05:47:45 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9E37C2BABE0; Tue, 28 Dec 2010 00:47:43 -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 vTfMWZTjQJN9; Tue, 28 Dec 2010 00:47:43 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 226D52BABB7; Tue, 28 Dec 2010 00:47:43 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id BFA331457BB; Tue, 28 Dec 2010 06:47:33 +0100 (CET) Date: Tue, 28 Dec 2010 06:35:00 -0000 From: Joel Brobecker To: Andrew Burgess Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH] MI disassemble opcode support Message-ID: <20101228054733.GC2596@adacore.com> References: <4D08C355.9010809@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D08C355.9010809@broadcom.com> 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-12/txt/msg00491.txt.bz2 > 2010-12-10 Andrew Burgess > > * disasm.c (dump_insns): Build the opcodes into a stream so > that we can dump them as a field for MI. > > gdb/doc/ > > 2010-12-10 Andrew Burgess > > * gdb.texinfo: Update to reflect changes in mi/mi-cmd-disas.c > > gdb/mi/ > > 2010-12-10 Andrew Burgess > > * mi-cmd-disas.c (mi_cmd_disassemble): Allow mode to control > dumping of instruction opcodes. > > gdb/testsuite/ > > 2010-12-10 Andrew Burgess > > * gdb.mi/mi-disassemble.exp, gdb.mi/mi2-disassemble.exp: Update > expected output to reflect changes in gdb/mi/mi-cmd-disas.c and > add new tests to check opcode dumping works. Pre-approved with the following changes made: I think a NEWS entry might be worthwhile (this can be treated as a separate patch - maybe ask Eli if he thinks it's significant enough to be mentioned there). The alignment of your ChangeLog entries is incorrect. Everything should be aligned on a tabulation. Therefore: * gdb.mi/mi-disassemble.exp, gdb.mi/mi2-disassemble.exp: Update expected output to reflect changes in gdb/mi/mi-cmd-disas.c and add new tests to check opcode dumping works. A general comment: The ChangeLog should explain the WHAT. If you feel that you need to explain the WHY, that comment should be in the code. For instance: > + const char *spacer = ""; > + > + /* Temporary stream for building up the opcodes */ You can put the reason why you are building the opcode in a stream here instead of inside the ChangeLog. > + fprintf_filtered(opcode_stream->stream, "%s%02x", > + spacer, (unsigned)data); > + spacer = " "; Formatting: space before the first '(', and also space after the unsigned cast. I know the second one is not your doing, but might as well fix it. > error > ("mi_cmd_disassemble: Usage: [-f filename -l linenum [-n howmany]] [-s startaddr -e endaddr] [--] mixed_mode."); This error message need to be updated (replace "mixed_mode" by "mode"). > + /* Convert the mode into a set of disassembly flags */ Period and 2 spaces at the end of the sentence. -- Joel