Jump to content

  • Log In with Google      Sign In   
  • Create Account


#ActualJeffreyp

Posted 07 March 2013 - 07:28 AM

That's not a pool allocator, it's a stack allocator.

 

In the destructor you shouldn't be deleting both memory and at. at is inside memory (and at wasn't created with new), it's already deleted when you delete memory.

The atAmount variable is always zero. What's it for?
Instead of "atAmount + amount > size - 1", you should be using "at + amount > memory + size".
 
You should merge the allocate method into the constructor, so it's created like:
PoolAllocator allocator(1000);
At the moment, someone could call allocate twice, and your class would cause a memory leak. 


The allocator should probably be non-copyable, because at the moment if I write PoolAllocator other = allocator; your code will crash.
e.g.

class PoolAllocator
{
public:
...
private:
    PoolAllocator(const PoolAllocator&);//not implemented
    PoolAllocator& operator=(const PoolAllocator&);//prevents accidentally copying
...
};

 

If the free method does nothing, it shouldn't exist. At the moment, it will only fool your users into thinking that it's necessary, or that they are actually freeing memory.

 

The obtain function does not take alignment into account. If someone obtains a char and then an int, the int will be misaligned.

 

Thanks for the response!

Some really informative things in there.

 

 

That's not a pool allocator, it's a stack allocator.

 

Yes you're right. I read it up on wikipedia and understand it now.

 

 

The atAmount variable is always zero. What's it for?
Instead of "atAmount + amount > size - 1", you should be using "at + amount > memory + size".

 

atAmount actually does the same as

at + amount > memory + size

 

But shouldn't be

at + amount > memory + size - 1

 

Because if

at + amount == memory + size

it would crash at

at+= amount
right?

 

 

If the free method does nothing, it shouldn't exist. At the moment, it will only fool your users into thinking that it's necessary, or that they are actually freeing memory.

 

Actually I don't know how I should implement this method. Can you give me some tips?

 

Thanks.


#2Jeffreyp

Posted 07 March 2013 - 07:27 AM

That's not a pool allocator, it's a stack allocator.

 

In the destructor you shouldn't be deleting both memory and at. at is inside memory (and at wasn't created with new), it's already deleted when you delete memory.

The atAmount variable is always zero. What's it for?
Instead of "atAmount + amount > size - 1", you should be using "at + amount > memory + size".
 
You should merge the allocate method into the constructor, so it's created like:
PoolAllocator allocator(1000);
At the moment, someone could call allocate twice, and your class would cause a memory leak. 


The allocator should probably be non-copyable, because at the moment if I write PoolAllocator other = allocator; your code will crash.
e.g.

class PoolAllocator
{
public:
...
private:
    PoolAllocator(const PoolAllocator&);//not implemented
    PoolAllocator& operator=(const PoolAllocator&);//prevents accidentally copying
...
};

 

If the free method does nothing, it shouldn't exist. At the moment, it will only fool your users into thinking that it's necessary, or that they are actually freeing memory.

 

The obtain function does not take alignment into account. If someone obtains a char and then an int, the int will be misaligned.

 

Thanks for the response!

Some really informative things in there.

 

 

That's not a pool allocator, it's a stack allocator.

 

Yes you're right. I read it up on wikipedia and understand it now.

 

 

The atAmount variable is always zero. What's it for?
Instead of "atAmount + amount > size - 1", you should be using "at + amount > memory + size".

 

atAmount actually does the same as

at + amount > memory + size

 

But shouldn't be

at + amount > memory + size - 1

 

Because if

at + amount == memory + size

it would crash at

at+= amount
right?

 

 

If the free method does nothing, it shouldn't exist. At the moment, it will only fool your users into thinking that it's necessary, or that they are actually freeing memory.

 

Actually I don't know how I should implent this method. Can you give me some tips?

 

Thanks.


#1Jeffreyp

Posted 07 March 2013 - 07:25 AM

That's not a pool allocator, it's a stack allocator.

 

In the destructor you shouldn't be deleting both memory and at. at is inside memory (and at wasn't created with new), it's already deleted when you delete memory.

The atAmount variable is always zero. What's it for?
Instead of "atAmount + amount > size - 1", you should be using "at + amount > memory + size".
 
You should merge the allocate method into the constructor, so it's created like:
PoolAllocator allocator(1000);
At the moment, someone could call allocate twice, and your class would cause a memory leak. 


The allocator should probably be non-copyable, because at the moment if I write PoolAllocator other = allocator; your code will crash.
e.g.

class PoolAllocator
{
public:
...
private:
    PoolAllocator(const PoolAllocator&);//not implemented
    PoolAllocator& operator=(const PoolAllocator&);//prevents accidentally copying
...
};

 

If the free method does nothing, it shouldn't exist. At the moment, it will only fool your users into thinking that it's necessary, or that they are actually freeing memory.

 

The obtain function does not take alignment into account. If someone obtains a char and then an int, the int will be misaligned.

 

Thanks for the response!

Some really informative things in there.

 

That's not a pool allocator, it's a stack allocator.

 

Yes you're right. I read it up on wikipedia and understand it now.

 

The atAmount variable is always zero. What's it for?
Instead of "atAmount + amount > size - 1", you should be using "at + amount > memory + size".

 

atAmount actually does the same as

at + amount > memory + size

 

But shouldn't be

at + amount > memory + size - 1

 

Because if

at + amount == memory + size

it would crash at

at+= amount
right?

 

If the free method does nothing, it shouldn't exist. At the moment, it will only fool your users into thinking that it's necessary, or that they are actually freeing memory.

 

Actually I don't know how i should implent this method. Can you give me some tips?

 

Thanks.


PARTNERS