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

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: Text/Plain, Size: 2388 bytes --]

On Tuesday, December 14, 2010 02:35:58 Joel Brobecker wrote:
> > +	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...

i dont understand ... isnt this the whole purpose of the first clause of the 
for statement ?  initializing things once that dont change inside of the loop.  
by this same logic, you could say i should hoist the nr_map assignment out 
too.  or is the multiple initializing assignments undesirable ?

> > +	    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.

i dont really like that idea

> 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);

but this idea is good, so i'll implement it

> > +		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?

it's taken largely unchanged from the OPTION_MEMORY_INFO case block just above 
my new block.  i guess my new code could review the types and do it right.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]