Re: [ECOS] New memory allocation debug feature

[Andrew Lunn <andrew at lunn dot ch>]

> New patch attached.

Thanks

I started to look at the patch, but decided there are a number of
things i don't like.

There should be a prototype for cyg_memalloc_alloc_fail() in one of
the normal header file so that user code can use it. You put it into
common.hxx which is wrong. Application code is not supposed to use the
C++ API. I would add something like

__externC void cyg_memalloc_alloc_fail(char * file, int line, cyg_int32 size)
__THROW __CYG_ATTRIBUTE_WEAK;

to services/memalloc/common/current/include/kapi.h. This should not be
conditional, since the application code should be able to have the
function even if eCos is not configured to use it.

But this then leads to a problem with the code in common.hxx. You make
the function an empty inline function when
CYSEM_MEMALLOC_INVOKE_OUT_OF_MEMORY is disabled, assuming the
optimizer will optimize out the function call and the test to see if
the ptr is NULL. But you cannot have the function both __externC void
and inline void. The compiler should complain. Im also not sure the
optimizer is that smart. And since the empty inline is the normal
case, we don't want none optimal code here. So i would suggest putting
something like this in common.hxx

#ifdef CYSEM_MEMALLOC_INVOKE_OUT_OF_MEMORY
#define CYG_MEMALLOC_FAIL_TEST( ptr, size)          \
   CYG_MACRO_BEGIN                                  \
   if ( NULL == ptr) {                              \
        cyg_malloc_fail(__FILE__, __LINE__, size ); \
   }                                                \
   CYG_MACRO_END
#else
#define CYG_MEMALLOC_FAIL_TEST( ptr, size)  CYG_EMPTY_STATEMENT
#endif        

Then change all your tests to calls to the MACRO.

You debug.cxx should really debug.c You want a C function so you might
as well use the C compiler not the C++ compiler. The function needs
updating the take the paramters i've suggested. You also didn't make
it a weak funcion like i asked.

A few other minor things. CYSEM_MEMALLOC_INVOKE_OUTOFMEMORY should be
CYSEM_MEMALLOC_INVOKE_OUT_OF_MEMORY, following the same naming
convention as cyg_memalloc_alloc_failed. ie "_" between words.

Also its normal to not have lines longer than around 78 charactors in
eCos files. Both your changes to memalloc.cdl and ChangeLog had lines
which were too long.

> 
> > > You should try emacs. 
> 
> I use emacs all the time, but from different computers, so I find that I don't 
> want to spend the time configuring it.

One little trick i've seen people use is to checking all their .rc
files into CVS. When change something on one machine, check it in and
then cvs up on all the other machines. Alternativly, just have you
home directory on a network drive which all machines use.

        Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss