Jump to content

  • Log In with Google      Sign In   
  • Create Account

#ActualHodgman

Posted 07 March 2013 - 07:06 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.


#3Hodgman

Posted 07 March 2013 - 07:05 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.


#2Hodgman

Posted 07 March 2013 - 07:04 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.


#1Hodgman

Posted 07 March 2013 - 07:02 AM

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

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.


PARTNERS