[interfacekit] Re: BBlockCache bug

On 2003-08-27 at 10:43:40 [+0200], burton666@xxxxxxxxx wrote:
> About BBlockCache ... I guess I caught the destructor bug:
> 
> BBlockCache::~BBlockCache()
> {
>   delete[] (int8*)fCache;
> }
> 
> 
> since fCache is allocated as
> 
> fCache = (void *)new _Block[CacheSize];
> 
> it should be deleted as
> 
> delete[] (_Block *)fCache;
> 
> 
> I can't fix it now since I am on a windows machine (at work :P), if no one 
> is faster I see if I can fix it within this week.

I committed the fix.

> A more important note: The current BBlockCache implementation just use 
> malloc or new... shouldn't it use a proper memory allocator?

I think it can't without breaking compatibility. The class is documented to 
use alloc()/new, so we can't change this.

> Yeah, I know the last BlockCache commit shows my name, but it was just a 
> cleanup. The original implementation was by Massimiliano Origgi.

I guess, that's why his name is listed as author in the file. ;-P

Anyway, what troubles me much more is, that I had a quick glance at the 
implementation, and I'm not very happy with it. Aside from that the 
destructor doesn't free the unused blocks now, it's more 
complicated/inefficient than it need be.

For one I don't see the reason for this casting orgy -- fCache is void* 
instead of _Block*, as it should be -- but I actually don't even see why to 
use the _Block structures at all. In Be's header fCache is void**, that is 
it is an array with pointers to the blocks (the actual chunks of memory). 
The fMark member should certainly be the number of used blocks, or, in other 
words, the separation mark for used and unused block, given that one keeps 
the used blocks at the beginning and the free ones at the end (simply done 
by Get()ing blocks at fCache[fMark] and swapping the pointer of a Save()d 
block with the last used one).

Furthermore I would recommend to check whether locking was successful. For 
benaphore style lockers, things don't work too well when operating on an 
already destroyed object, since they first write to freed memory and ask 
questions later, but it would give a bit more safety anyway.

CU, Ingo

PS: The class isn't in the build anyway.

Other related posts: