Re: FW: [PATCH][STEP 2] ctype changes for removing locale structures

[Craig Howland <howland at LGSInnovations dot com>]

On 02/09/2018 02:32 PM, Corinna Vinschen wrote:
> On Feb  9 19:09, Jaap de Wolff wrote:
> > Van: Jaap de Wolff [mailto:info@jasoon.nl]
> > > Van: newlib-owner@sourceware.org [mailto:newlib-owner@sourceware.org]
> > > On 02/09/2018 10:06 AM, Jaap de Wolff wrote:
> > > > modify ctype not to use __locale_ctype_ptr (== __CTYPE_PTR) or
> > > > __locale_ctype_ptr_l To do this al inline functions are removed, and
> > > > the source implementations test hard coded values of the given
> > > > characters
> > > >
> > > >
> > > > ================================== PATCH ================= diff --
> > > git
> > > > a/newlib/libc/ctype/isalnum.c b/newlib/libc/ctype/isalnum.c index
> > > > d926f97b7..6d3864114 100644
> > > > --- a/newlib/libc/ctype/isalnum.c
> > > > +++ b/newlib/libc/ctype/isalnum.c
> > > > @@ -46,5 +46,9 @@ No OS subroutines are required.
> > > >    int
> > > >    isalnum (int c)
> > > >    {
> > > > +#ifdef _REENT_SMALL
> > > > +	return (isdigit(c) | islower(c) | isupper(c)); #else
> > > >    	return(__CTYPE_PTR[c+1] & (_U|_L|_N));
> > > > +#endif
> > > >    }
> > > I would think that we should be able to do better than this, as this
> > > method is taking a half step backwards for some of the functions, such
> > > as isalnum().  That is, it potentially loses the efficiency of doing
> > > one table lookup plus one OR, as it needs to do N table lookups and
> > > N+N-1 ORs (where N appears to be from 1-3).  I say "potentially"
> > > because maybe the compiler would end up reducing it to the
> > > 1+1 case, but maybe not, too (depending on compiler, compiler
> > > 1+settings). Before
> > > locale was added, isalnum(), for example, was
> > >
> > > return(__ctype_ptr__[c+1] & (_U|_L));
> > >
> > > I'm not sure what the right thing is now (still __ctype_ptr__?), but
> > > the same basic thing ought to work, knowing the right thing to put in.
> > > But for that matter, why not in the REENT_SMALL case simply redefine
> > > __CTYPE_PTR to point to the right thing?  Then all these other source
> > > file changes become unnecessary. It seems reasonable to expect this to
> > > work, and it would be a cleaner solution (since the is*.c files remain
> > > untouched, without #ifs).  The stated purpose of the patch is to stop
> > > ctype from using __locale_ctype_ptr, but ctype.h is where it is defined to be __CTYPE_PTR--so just define it differently.
> > >
> > > Craig
> > Craig,
> > In basic the intention of my patches is to make to memory footprint for embedded environment small.
> > I talked about locale, and a part of the locale definition is a lookup table with the characteristics of each character.
> > Of course it is possible to move the lookup table outside the locale, but then the lookup table still takes 257 bytes of memory.
> >
> > In the beginning my intention was just to move the __global_locale table from ram to rom, and a reaction was that that was not enough when using embedded processors with 2K or even 1K of ROM and 128 bytes of RAM.
> >
> > I do think that for the intended users of the REENT_SMALL computation speed (efficiency) is less important as memory.
> > So I replaces all usage of a lookup table by a (kind of) computation.
> I'm more with what Craig said.  A pointer to a character table has been
> used since newlib started in the 90s.  We never had complaints for using
> a single character table before.
>
>
> Corinna
Jaap has a good point, since the patch does actually totally get rid of the 
table and uses comparisons.  So while we could get more instructions in a few 
functions, it seems unlikely they would add up to the table, and that size could 
be saved even for the most minimal application.  In addition, the macro table 
lookup plus operation is likely larger than a function call, so the more an app 
calls them it is likely that, relatively speaking, more size is saved.

But to continue the discussion, a big-picture question/issue.  Is coupling it to 
REENT_SMALL the best approach?  Would ditching the table based on 
PREFER_SIZE_OVER_SPEED, as is done for a number of the string functions, make 
more sense?  Or maybe REENT_SMALL && PREFER_SIZE_OVER_SPEED?  A new option?  (In 
case this were somehow considered too radical.  For example, you might want your 
speed and be able to put the table into ROM and so save RAM space (as long as 
there are not too many macor calls).)

Another possible approach would be to say that the table and the macros should 
be kept, at the same time that the functions use PREFER_SIZE_OVER_SPEED to not 
use the table.  The user then has complete control over what they get on an 
individual call basis rather than a library basis.  If you want some speed and 
can afford the size of the table, just default to getting the macros.  If you 
don't want that, use whatever method you want to defeat the macros (#undef, 
escaping, etc.).

Craig

Craig