Re: [PATCH 3/3] objdump: allow the disassembler colors to be customized

[Nick Clifton <nickc@redhat.com>]

Hi Andrew,

> I don't know if this style of environment encoded settings will be
> acceptable or not.  

If we are going to accept an environment variable for patch 2/3 then
I see no reason why it cannot be extended here.

> The alternative would be some kind of
> configuration file system. 

Meh - of the two I prefer environment variables over configuration files.
Environment variables are easier to change in a nested fashion, configuration
files are not.

> My inspiration for the approach I propose
> here is the LS_COLORS variable used by `ls`.

Agreed - there is precedence for this kind of behaviour.

> +<mode_selection> ::= "OFF" | "B" | "X"

Shouldn't the modes be "off", "on" and "extended" ?

> +<style_item> ::= <style_code> "=" <escape_string>

Your syntax description does not document "<escape string>".
It really should do so, or else provide a reference to
where it is documentede.

> +<style_code> ::= "ad" | "ao" | "as" | "cm" | "im"
> +               | "mn" | "rg" | "sm" | "sy" | "tx"

I am a big fan of long descriptive names.  It makes things
much easier to read.  So by all means, allow shortened versions
if you want, but it would be really nice to support names like
"registers", "addresses", "symbols" etc.



> +      /* The defaults colors for --disassembler-color=color.  */
> +      disasm_styles[(unsigned int) dis_style_address] = "35";
> +      disasm_styles[(unsigned int) dis_style_address_offset] = "35";

You might as well replace these magic numbers (eg 35) with #define'd
constants.  That would be more descriptive.  (Assuming that the defined
names are color names of course).


Cheers
   Nick