Re: [PATCH] Reduce RAM usage of floating point conversion functions

[Corinna Vinschen <vinschen at redhat dot com>]

On Jun 10 12:08, Federico Terraneo wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 06/10/2013 11:29 AM, Freddie Chopin wrote:
> > W dniu 2013-06-10 11:12, Federico Terraneo pisze:
> >> I did think about that, but this moves 256 bytes of .bss to 256 
> >> bytes of .rodata, i.e. it would take up that amount of FLASH on
> >> a microcontroller, and the table is mostly filled with zeros,
> >> which still seems a waste. Here is a patch that goes in that
> >> direction: if PREFER_SIZE_OVER_SPEED, __OPTIMIZE_SIZE__ or
> >> _SMALL_HEXDIG are defined, the table is still replaced with a
> >> function, otherwise the table is made const (but not static,
> >> since it's used also in another file). In this way in both cases
> >> 256 bytes of RAM are saved, and in the former also 256 bytes of
> >> FLASH.
> > 
> > Great!
> > 
> > But one thing - is the patch correct? You define a macro 
> > __check_hexdig(), and in the code you use __get_hexdig(), which I 
> > cannot find.

Thanks for the review, Freddie!

> Thank you for noticing it. At one point I changed the function name,
> thought I've changed the name everywhere but tried to compile the
> individual files for errors to be double sure, relying on the compiler
> to check for such errors. However I forgot that in C you can call a
> function without having first declared it (I mostly write in C++), so
> that slipped through anyway.
> 
> New patch attached.
> 
> > 
> > BTW - could you check what's the size of the new function you 
> > introduced (__hexdig_fun())? If it's not smaller than this 256B 
> > array than this is not a size optimization (;
> 
> I've tested it compiling for the thumb2 intruction set (Cortex-M3) and
> it's 52 bytes, while for the ARM instruction set is 64 bytes.

Federico, given Freddies review the patch is fine.  However, what's
missing is a ChangeLog entry.  Can you please provide one as well?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat