Re: [PATCH] MI disassemble opcode support

[Joel Brobecker <brobecker at adacore dot com>]

> 2010-12-10  Andrew Burgess  <aburgess@broadcom.com>
> 
>           * 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  <aburgess@broadcom.com>
> 
>           * gdb.texinfo: Update to reflect changes in mi/mi-cmd-disas.c
> 
> gdb/mi/
> 
> 2010-12-10  Andrew Burgess  <aburgess@broadcom.com>
> 
>           * mi-cmd-disas.c (mi_cmd_disassemble): Allow mode to control
>             dumping of instruction opcodes.
> 
> gdb/testsuite/
> 
> 2010-12-10  Andrew Burgess  <aburgess@broadcom.com>
> 
>           * 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