Re: callback functionality for dyanmic memory allocation

[Jonathan Larmour <jifl at eCosCentric dot com>]

NavEcos wrote:
> > Also, how about writing a test or two?  These are the best examples
> > we can provide the public of anything that gets added.
> >
> > Thanks.  (I'll let Jonathan comment on the patch as a whole)
>
> I am using the addtional functionality already - but before I publish 
> examples, I think it would be best to see what eCosCentric thinks.  I expect 
> there will be some discussion about changes -> once the additional API's are 
> solidified I'll be happy to produce examples.
Well, okay you asked for it ;-). I'm not convinced of the need to have the 
callback functions being a zillion different types, most of which are 
pretty similar (pool, ptr, size most times).

I think it may make for a more understandable interface if it's a single 
type, with an "operator" enum indicating what function is being performed. 
I would even go as far as suggesting, since the primary use of this is 
debugging, that there even only be one function that's called full stop. 
i.e. instead of 8 statics, there's just the one. If the user's callback 
isn't interested in a particular operation, it can just drop out and 
ignore it.

This callback could be of type (e.g. for dlmalloc):

typedef void Cyg_mempool_dl_callback_fn( enum Cyg_Mempool_dl_op op, 
Cyg_Mempool_dlmalloc *pool, const cyg_uint8 *base_ptr, cyg_int32 size, 
cyg_int32 *newsize );

Obviously, unused arguments for the particular op can just be ignored.

I think this would greatly simplify this for users. Having so many 
different callbacks and types seems cumbersome.

I'm not clear on why for e.g. fn_pre_realloc_cb you want to take the 
_address_ of alloc_ptr. alloc_ptr itself should do. Ditto for other 
cyg_uint8**.

You've left in a test_fun in dlmalloc.

This is definitely large enough for a copyright assignment, so that's 
required before it can be included.

Just when it comes to code layout, I think it's superior that instead of e.g.

#ifdef CYGSEM_MEMALLOC_ALLOCATOR_FIXED_CALLBACK
    cyg_uint8
      *pRet;

    if ( fn_pre_alloc_cb != NULL ) {
       (*fn_pre_alloc_cb) (this);
    }

    pRet = mypool.try_alloc( 0 );

    if ( fn_post_alloc_cb != NULL ) {
       (*fn_post_alloc_cb) (this, &pRet);
    }
    return pRet;
#else
     return mypool.try_alloc( 0 );
#endif

we have:

    cyg_uint8 *pRet;

#ifdef CYGSEM_MEMALLOC_ALLOCATOR_FIXED_CALLBACK
    if ( fn_pre_alloc_cb != NULL ) {
       (*fn_pre_alloc_cb) (this);
    }
#endif

    pRet = mypool.try_alloc( 0 );

#ifdef CYGSEM_MEMALLOC_ALLOCATOR_FIXED_CALLBACK
    if ( fn_post_alloc_cb != NULL ) {
       (*fn_post_alloc_cb) (this, &pRet);
    }
#endif
    return pRet;

It makes more sense to me.

Feel free to add yourself as a contributor at the top BTW. It's up to you.

> Also, to help understand it: the dlmalloc and variable heaps have nearly 
> identical APIs but the fixed alloc is a bit different.
>
> If you have any questions, feel free to ask.
Done :-). To echo Gary, a test would be good. Or failing that, just a 
little guide on how to use it which can go in the package's doc directory. 
ASCII text would be good enough since there isn't already an SGML doc to 
hook into (but if you wanted to start one that would naturally be fine too 
:-)).

Jifl
--
eCosCentric       http://www.eCosCentric.com/       <info@eCosCentric.com>
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine