Zach Welch wrote:
> On Fri, 2009-06-19 at 20:31 -0400, Duane Ellis wrote:
>
>> duane> FYI - I committed several cygwin specific printf() warning fixes.
>> duane> Simple cast to fix
>> duane> these where causing "-Werror" failures on cygwin.
>>
>> zach> I was just about to post some patches to show how to fix all of these
>> zach> correctly, as casts are not the right way to do it.
>>
>> In some cases, I can agree, but in others - I don't agree. A cast is by
>> far the most simplest solution.
>>
>> For instance, look at this:
>>
>> LOG_WARNING("writing %d bytes only - as image section is %d
>> bytes and bank is only %d bytes", \
>> (int)(c->base + c->size - run_address),
>> (int)(run_size), (int)(c->size));
>>
>> (a) look at what is being printed
>> (b) the possible ranges of numbers
>> (c) the number of parameters involved in the expressions
>>
>> At some point - having to dig back 20 to 30 -lines- to *random* places -
>> because some variables are:
>>
>> (1) function parameters
>> (2) defined at the top most block
>> (3) defined locally to the local block
>> (4) defined a few lines down from the top most block
>> (5) often simplistic 'i/j/k' type vars that are reused because they are
>> handy.
>> (6) By the time I personally dig through the above, and determine all
>> types involved
>> (7) Then understand the underlying arithmetic integer promotion order...
>> *note* this set of equations are simple...
>> (8) a cast to a basic type is truly a very *simple* solution,
>> (9) a cast to a basic type in this case is the K.I.S.S. solution.
>>
>> Either that - or we *need* a different solution here, the above is absurd.
>>
>> Comments?
>>
>
> The simple solution is not the correct one in this case. The problem
> is that your assumptions about ranges are not portable; that is the
> point in using these macros.
>
>
====
You miss an important point, it is *NOT* the possible range of values
the *TYPE* may take on.
It is the range of values the **ACTUAL*DATA** might take on that is
important here.
Consider:
uint32_t x;
void funny_function( uint32_t );
for( x = 0 ; x < 10 ; x++ ){
printf(" X = %d, Calling funny function\n", (int)(x));
funny_function( x ) ;
}
In the above, I have several choices, (a) I could have used an "int"
variable - then *cast* it to a uint32_t at the function call (note here:
I am partial to x as a loop control variable, others like i/j/k) or -
because (b) the value will never be negative, I just choose 'uint32_t'
because it was convient.
Is the printf() statement *WRONG* - or is the printf() statement
*CORRECT* in this senario.
====
If we had to fix this, what would we do to fix this:
Today - we do not have macros, or types like GDB/GCC/GAS.
GDB for instance uses CORE_ADDR - and has infrastructure behind it.
Here, under OpenOCD we generally:
(1) assume that all embedded openocd targets are *32bit* at most...
(2) assume all host platforms are at least 32bit host platforms
(3) and thus, target addresses are generally equal to - or smaller then
the host "unsigned integers"
(4) In most, if not all of the "u32" cases, the format string specifies
"%08x" or equal.
Are these assumptions *wrong*? If so, we have major other problems we
must deal with.
====
There are numerous examples of this where - for instance the code does
the following:
LOG_ERROR("Verfication error address 0x%08x, read back 0x%02x,
expected 0x%02x", address + wrote + i, readback[i], chunk[i]);
In this specific case, the *correct* fix is something else.
The "address" parameter really needs a *function* that handles the width
value. Otherwise, a "64bit" target will not work. We are not proposing
to fix these situations today are we? Doing so would really be better
served by a function something like:
char *target_address_as_hex( CORE_ADDR address_value );
One can 'question' the two 8bit values printed, allow me to make my
point another way.
====
Here is another example:
LOG_INFO( "device id = 0x%08x (manuf 0x%03x dev 0x%02x, ver 0x%03x)",
device_id, (device_id>>1)&0x7ff, (device_id>>12)&0xff,
(device_id>>20)&0xfff );
The format strings alone tell me that a 32bit unsigned *INTEGER* is
sufficient.
As does the math operators.
Since our *basic* assumption of the host platform is at least a "32bit
unsigned int"
A simple cast is valid here.
Are you suggesting that our *basic* host assumption is wrong?
====
duane> 9) a cast to a basic type in this case is the K.I.S.S.
solution.duane>
zach> I don't care one white about KISS if the code is wrong.
Is this a pedantic position/point you are trying to make?
The code today has *major* problems if/when we have a 64bit target.
The code is more *wrong* when we have 32bit hosts and 64bit targets.
-Duane.
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
