Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#7030 - Adds the printf format for pointers (%p or %P)

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Saturday, 14 April 2007, 17:40 GMT+2
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This adds the printf format for pointers. Use %p or %P.

Example:

int *p;
printf("Int value at %p is %d", p, *p);

Also corrected the code indentation and factored out duplicated code.
(application/octet-stream)    printf_pointer.patch (7.5 KiB) 
 firmware/common/sprintf.c |  238 +++++++++++++++++++++++-----------------------
 1 file changed, 123 insertions(+), 115 deletions(-)

This task depends upon

Closed by  Peter D'Hoye (petur)
Monday, 16 April 2007, 13:33 GMT+2
Reason for closing:  Rejected
Additional comments about closing:  the addition to the bin size just isn\'t worth it
Comment by Alexander Levin (fml2) - Saturday, 14 April 2007, 21:30 GMT+2
With this patch, %x and %p produce lower case hex and %X and %P produce upper case hex.

Also fixed copy&paste error.
(application/octet-stream)    printf_pointer2.patch (8 KiB) 
 firmware/common/sprintf.c |  246 ++++++++++++++++++++++++----------------------
 1 file changed, 130 insertions(+), 116 deletions(-)

Comment by Alexander Levin (fml2) - Saturday, 14 April 2007, 22:47 GMT+2
This also adds %i as a synonym for %d.
(application/octet-stream)    printf_pointer3.patch (8 KiB) 
 firmware/common/sprintf.c |  248 ++++++++++++++++++++++++----------------------
 1 file changed, 132 insertions(+), 116 deletions(-)

Comment by Peter D'Hoye (petur) - Sunday, 15 April 2007, 10:18 GMT+2
Unless there's any real use to this, this patch only increases binary size.
For example, H300 went from 414528 to 414868 (+340 bytes)

Apart from that, I did look at the patch and enhanced it a bit, so here's my version.
The inline is used because performance is important
(application/octet-stream)    format.patch (9.1 KiB) 
 firmware/common/sprintf.c |  262 +++++++++++++++++++++++++---------------------
 1 file changed, 143 insertions(+), 119 deletions(-)

Comment by Alexander Levin (fml2) - Sunday, 15 April 2007, 10:33 GMT+2
Removed %P since it doesn't seem to be in the printf spec. Making funcs inline would probably increase the binary size.

I've added the %p tag because I wanted to investigate how pointers are affected by IRAM attributes and found out that the tag isn't present in RB. Should I cast pointers to long to display them?
(application/octet-stream)    printf_pointer4.patch (8.9 KiB) 
 firmware/common/sprintf.c |  258 ++++++++++++++++++++++++----------------------
 1 file changed, 139 insertions(+), 119 deletions(-)

Comment by Alexander Levin (fml2) - Sunday, 15 April 2007, 16:04 GMT+2
BTW: to further reduce the binary size, %i (for int) can be removed. I introduced it to be as near as possible to the 'official' printf spec, but if RB doesn't need it (and I think it doesnt't) it can be removed.

@petur: why do you think performance in printf is so important? Could we trade a little bit of speed for binary size?
Comment by Alexander Levin (fml2) - Sunday, 15 April 2007, 22:10 GMT+2
Petur, I tried some combinations of code changes and you are right: the binary size increases. I don't undestand why but it does. So we can refuse on this change and use conversion to long instead.
Comment by Peter D'Hoye (petur) - Monday, 16 April 2007, 00:44 GMT+2
Well I give up on this. I tried to just write some parts of the SVN version nicer and more compact, but whatever I try, the binsize grows. Attached is my last try, but I'll probably close this patch anyway as it's grown pretty useless.
(application/octet-stream)    sprintf5.patch (9.5 KiB) 
 firmware/common/sprintf.c |  233 +++++++++++++++++++++++-----------------------
 1 file changed, 119 insertions(+), 114 deletions(-)

Comment by Alexander Levin (fml2) - Monday, 16 April 2007, 09:12 GMT+2
Same here. And agreed on closing. But it's weird, no? :-)

Loading...