Hello,

Today I decided that I want to gain some knowledge about memory management.

I don't know much about memory management so i thought it would be a nice "exercise" to try to create a pool allocator.

Actually everything is working as expected but I think I did something wrong.

Here's the code :

PoolAllocator.h

[source]

#pragma once


class PoolAllocator
{
public:
PoolAllocator(void);
~PoolAllocator(void);

void allocate(const size_t amount);
char *obtain(const size_t amount);
void free(void *mem);

private:

char *memory;
char *at;
int atAmount;
int size;

};

[/source]

PoolAllocator.cpp

[source]

#include "PoolAllocator.h"


PoolAllocator::PoolAllocator(void)
{
}


PoolAllocator::~PoolAllocator(void)
{
delete[] memory;

delete[] at;
}


void PoolAllocator::allocate(const size_t maxSize)
{
memory = new char[maxSize];
at = &memory[0];
atAmount = 0;
size = maxSize;
}

char *PoolAllocator::obtain(const size_t amount)
{
if(atAmount + amount > size - 1)
return nullptr;

at += amount;

return at;
}

void PoolAllocator::free(void *mem)
{

}

[/source]

main.cpp

[source]

#include <Windows.h>
#include <iostream>

#include "PoolAllocator.h"

class A
{
public:
int val;

};

class B
{
public:
int val;

};

int main()
{
PoolAllocator allocator;
allocator.allocate(1000);

A *a = (A*)allocator.obtain(sizeof(A));

a->val = 10;

std::cout << "A : " << a->val << std::endl;

B *b = (B*)allocator.obtain(sizeof(B));

b->val = 20;

std::cout << "A : " << a->val << std::endl << "B : " << b->val << std::endl;

system("pause");
return 0;
}

[/source]

My knowledge in memory management is really limited, so it could be that I made some 'horrible' mistake or something.

Basically I just want someone to check this code.

Thanks in advance !

If you're interested in creating a pool allocator (not a stack allocator, like Hodgman noted), I've written about that on my blog

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

[source]

at + amount > memory + size

[/source]

But shouldn't be

[source]

at + amount > memory + size - 1

[/source]

Because if [source]at + amount == memory + size[/source]

it would crash at [source] at+= amount [/source] 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.

If you're interested in creating a pool allocator (not a stack allocator, like Hodgman noted), I've written about that on my blog

Thanks I will check it out!

You should not have the -1 in the comparison. For example, if you allocate 1000 bytes and then attempt to obtain 1000 bytes, it should succeed. But as it is you can only obtain 999 bytes.

More importantly, you should be returning the value of 'at' before adding 'amount' to it, or you will overrun the end of the buffer.

Oké, thanks for the explanation

Your use of class member variables should take note of being a member variable by either:

this->at
or naming it

m_at; (m_ noting it is a 'member' variable)

You may also want to return a void* instead of char* since you are technically return a pointer to not a char but a random object (could be a GameObject*, int*, Texture*)

Thanks for your response.

Your use of class member variables should take note of being a member variable by either:

this->at
or naming it
m_at; (m_ noting it is a 'member' variable)

Isn't naming class members with m_ a bad habit? The reason I'm saying this is because a few months ago I had an internship and

I showed some c++ source code and they said that naming c++ class members with m_ is a bad habit from the C language. Just curious : )

You may also want to return a void* instead of char* since you are technically return a pointer to not a char but a random object (could be a GameObject*, int*, Texture*)

Oké, any other reasons I should use void* instead of char *? I saw both when I was googling around so I'm not really sure.

Thanks !

I showed some c++ source code and they said that naming c++ class members with m_ is a bad habit from the C language. Just curious : )

read the posts on this stackoverflow page for a discussion:

http://stackoverflow.com/questions/1228161/why-use-prefixes-on-member-variables-in-c-classes

i personally go for Jason Williams approach, have yet to see the bad side of prefixing.