Re: [RFA/i387] improve the output of 'info float' (was: "Re: gdb-patch mailing list")

[Joel Brobecker <brobecker at adacore dot com>]

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. <<Maintenance
update on 'info float'>> is just not informative at all, and ...

> +2010-01-02  Michael Baars  <development@codenamezero.org>
> +
> +	* 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