Re: [PATCH] sim: add --map-info option

[Joel Brobecker <brobecker@adacore.com>]

> 2010-12-01  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* sim-memopt.c (OPTION_MAP_INFO): Define.
> 	(memory_options): Handle --map-info.
> 	(memory_option_handler): Handle OPTION_MAP_INFO.

Some questions...

> +	for (memory = STATE_CORE (sd), nr_map = 0; nr_map < nr_maps; ++nr_map)

Wouldn't it make sense to hoist the assignment to memory out of
the for initial assignment? It's never changed during the loop...

> +	    if (nr_map <= io_map)
> +	      sim_io_printf (sd, "%s maps:\n",
> +			     (nr_map == read_map) ? "read" :
> +			     (nr_map == write_map) ? "write" :
> +			     (nr_map == exec_map) ? "exec" :
> +			     /*(nr_map == io_map) ?*/ "io");
> +	    else
> +	      sim_io_printf (sd, "??? (%u) maps:\n", nr_map);

I rather you used a case statement or series of if's, in this case
than assume that there are only 4 values <= io_map and thus if it's
not read_map or write_map or exec_map, then it's io_map.  How about:


    /* Return "read", or "write", or ... if valid nr_map.
       Otherwise return null;  */
    
    char *
    io_map_to_str (unsigned nr_map)
    {
      [...]

And then you can use that function to do:

        map_str = io_map_to_str (nr_map);
        if (map_str)
          sim_io_printf ("%s maps:\n", map_str);
        else
          sim_io_printf ("??? (%u) maps:\n", nr_map);

> +	    do
> +	      {
> +		unsigned modulo;

Empty line after local variable declarations.

> +		sim_io_printf (sd, " map ");
> +		if (mapping->space != 0)
> +		  sim_io_printf (sd, "0x%lx:", (long) mapping->space);
> +		sim_io_printf (sd, "0x%08lx", (long) mapping->base);
> +		if (mapping->level != 0)
> +		  sim_io_printf (sd, "@0x%lx", (long) mapping->level);
> +		sim_io_printf (sd, ",0x%lx", (long) mapping->nr_bytes);
> +		modulo = mapping->mask + 1;
> +		if (modulo != 0)
> +		  sim_io_printf (sd, "%%0x%lx", (long) modulo);

I don't understand the necessity to cast everything to long. Can you
explain?

Thanks,
-- 
Joel