Expanding on my previous comment, the name
ImmutableMemoryPoolgives the idea that the objects allocated by the pool are immutable, which is a pattern used sometimes to allow sharing of immutable instances, but that is not the case here. You meant to say that your pool has a fixed size, so why nonot call itFixedSizePool?Instead of defining a custom
bytetype, use the standardstd::uint8_tfrom<cstdint>. Everyone is already familiar with it.m_SizeOfChunkHeaderandm_SizeOfChunkDataare constants, so you shouldn't have them wasting memory as class members. They are also currently mutable, which is dangerous. Either just use thesizeof()expression directly, which is fine, or make those twostatic consts or betterstatic constexprif you're aiming at C++11 and above.Type casting should be avoided as much as possible. C++ relies on a strong type system. But when dealing with raw memory some type casting is unavoidable. When you have to typecast, use the C++ cast operators.
static_castandreinterpret_castare your friends. They produce better compiler diagnostics and are also very verbose and colorful on purpose, so they stand out as a clear sign that the programmer is taking matters into his/her own hands, for the better or for worse.The
ifnesting inDealloc()is unnecessary. The first condition can just be flipped and early return. The second can also be simplified from an if-then-else to an if-then-return.std::coutis not meant for error logging. You can however usestd::cerrif you want printed error output.cerr, orSTDERR, is the standard C++ stream for error output. But you might also consider replacing the error logs by exceptions, to give more flexibility to users of your code.Why is
InitializeHeaders()protected? You're not planning to inherit from this class, judging by the lack of avirtualdestructor, so anything internal should beprivate. By the way, this is a bit of a personal style, but I find it more relevantinteresting to place theprivatesection of a class right at the end. My rationale is that implementation details should not be the first thing you see when you look at a class declaration.Your decision of returning a
shared_ptrof the allocated block is questionable. What if all I need is aunique_ptr? My opinion is that this level of memory management is much too low to be dealing with smart pointers. All the user of a memory allocator/pool cares about is a pointer to the allocated block and the block size in bytes, so perhaps your allocator should deal in terms of aBlocktype? But I'm stealing this idea:P, I strongly suggest watching this excellent presentation: "std::allocator Is to Allocation what std::vector Is to Vexation".Memory alignment is most certainly going to be an issue with your allocator. The first object pulled from the memory pool will be aligned, because operator
newreturns properly aligned memory, but if the size of the allocated chunk is not evenly divisible by the minimum alignment, the second allocation won't be. This might result in misaligned memory access errors. You will very likely experience that if you try using your allocator with an__m128vector type that requires 16bytes16 bytes of alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_ofandstd::aligned_storage. You can also allow the specification of a minimum alignment as an integer template parameter.Tidy up those spurious blank lines here-and-there. One blank line is more than enough to separate two distinct areas of code.
Expanding on my previous comment, the name
ImmutableMemoryPoolgives the idea that the objects allocated by the pool are immutable, which is a pattern used sometimes to allow sharing of immutable instances, but that is not the case here. You meant to say that your pool has a fixed size, so why no call itFixedSizePool?Instead of defining a custom
bytetype, use the standardstd::uint8_tfrom<cstdint>. Everyone is already familiar with it.m_SizeOfChunkHeaderandm_SizeOfChunkDataare constants, so you shouldn't have them wasting memory as class members. They are also currently mutable, which is dangerous. Either just use thesizeof()expression directly, which is fine, or make those twostatic consts or betterstatic constexprif you're aiming at C++11 and above.Type casting should be avoided as much as possible. C++ relies on a strong type system. But when dealing with raw memory some type casting is unavoidable. When you have to typecast, use the C++ cast operators.
static_castandreinterpret_castare your friends. They produce better compiler diagnostics and are also very verbose and colorful on purpose, so they stand out as a clear sign that the programmer is taking matters into his/her own hands, for the better or for worse.The
ifnesting inDealloc()is unnecessary. The first condition can just be flipped and early return. The second can also be simplified from an if-then-else to an if-then-return.std::coutis not meant for error logging. You can however usestd::cerrif you want printed error output.cerr, orSTDERR, is the standard C++ stream for error output. But you might also consider replacing the error logs by exceptions, to give more flexibility to users of your code.Why is
InitializeHeaders()protected? You're not planning to inherit from this class, judging by the lack of avirtualdestructor, so anything internal should beprivate. By the way, this is a bit of a personal style, but I find it more relevant to place theprivatesection of a class right at the end. My rationale is that implementation details should not be the first thing you see when you look at a class declaration.Your decision of returning a
shared_ptrof the allocated block is questionable. What if all I need is aunique_ptr? My opinion is that this level of memory management is much too low to be dealing with smart pointers. All the user of a memory allocator/pool cares about is a pointer to the allocated block and the block size in bytes, so perhaps your allocator should deal in terms of aBlocktype? But I'm stealing this idea:P, I strongly suggest watching this excellent presentation: "std::allocator Is to Allocation what std::vector Is to Vexation".Memory alignment is most certainly going to be an issue with your allocator. The first object pulled from the memory pool will be aligned, because operator
newreturns properly aligned memory, but if the size of the allocated chunk is not evenly divisible by the minimum alignment, the second allocation won't be. This might result in misaligned memory access errors. You will very likely experience that if you try using your allocator with an__m128vector type that requires 16bytes alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_ofandstd::aligned_storage. You can also allow the specification of a minimum alignment as an integer template parameter.Tidy up those spurious blank lines here-and-there. One blank line is more than enough to separate two distinct areas of code.
Expanding on my previous comment, the name
ImmutableMemoryPoolgives the idea that the objects allocated by the pool are immutable, which is a pattern used sometimes to allow sharing of immutable instances, but that is not the case here. You meant to say that your pool has a fixed size, so why not call itFixedSizePool?Instead of defining a custom
bytetype, use the standardstd::uint8_tfrom<cstdint>. Everyone is already familiar with it.m_SizeOfChunkHeaderandm_SizeOfChunkDataare constants, so you shouldn't have them wasting memory as class members. They are also currently mutable, which is dangerous. Either just use thesizeof()expression directly, which is fine, or make those twostatic consts or betterstatic constexprif you're aiming at C++11 and above.Type casting should be avoided as much as possible. C++ relies on a strong type system. But when dealing with raw memory some type casting is unavoidable. When you have to typecast, use the C++ cast operators.
static_castandreinterpret_castare your friends. They produce better compiler diagnostics and are also very verbose and colorful on purpose, so they stand out as a clear sign that the programmer is taking matters into his/her own hands, for the better or for worse.The
ifnesting inDealloc()is unnecessary. The first condition can just be flipped and early return. The second can also be simplified from an if-then-else to an if-then-return.std::coutis not meant for error logging. You can however usestd::cerrif you want printed error output.cerr, orSTDERR, is the standard C++ stream for error output. But you might also consider replacing the error logs by exceptions, to give more flexibility to users of your code.Why is
InitializeHeaders()protected? You're not planning to inherit from this class, judging by the lack of avirtualdestructor, so anything internal should beprivate. By the way, this is a bit of a personal style, but I find it more interesting to place theprivatesection of a class right at the end. My rationale is that implementation details should not be the first thing you see when you look at a class declaration.Your decision of returning a
shared_ptrof the allocated block is questionable. What if all I need is aunique_ptr? My opinion is that this level of memory management is much too low to be dealing with smart pointers. All the user of a memory allocator/pool cares about is a pointer to the allocated block and the block size in bytes, so perhaps your allocator should deal in terms of aBlocktype? But I'm stealing this idea:P, I strongly suggest watching this excellent presentation: "std::allocator Is to Allocation what std::vector Is to Vexation".Memory alignment is most certainly going to be an issue with your allocator. The first object pulled from the memory pool will be aligned, because operator
newreturns properly aligned memory, but if the size of the allocated chunk is not evenly divisible by the minimum alignment, the second allocation won't be. This might result in misaligned memory access errors. You will very likely experience that if you try using your allocator with an__m128vector type that requires 16 bytes of alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_ofandstd::aligned_storage. You can also allow the specification of a minimum alignment as an integer template parameter.Tidy up those spurious blank lines here-and-there. One blank line is more than enough to separate two distinct areas of code.
A few things complementing the other answers:
Expanding on my previous comment, the name
ImmutableMemoryPoolgives the idea that the objects allocated by the pool are immutable, which is a pattern used sometimes to allow sharing of immutable instances, but that is not the case here. You meant to say that your pool has a fixed size, so why no call itFixedSizePool?Instead of defining a custom
bytetype, use the standardstd::uint8_tfrom<cstdint>. Everyone is already familiar with it.m_SizeOfChunkHeaderandm_SizeOfChunkDataare constants, so you shouldn't have them wasting memory as class members. They are also currently mutable, which is dangerous. Either just use thesizeof()expression directly, which is fine, or make those twostatic consts or betterstatic constexprif you're aiming at C++11 and above.Type casting should be avoided as much as possible. C++ relies on a strong type system. But when dealing with raw memory some type casting is unavoidable. When you have to typecast, use the C++ cast operators.
static_castandreinterpret_castare your friends. They produce better compiler diagnostics and are also very verbose and colorful on purpose, so they stand out as a clear sign that the programmer is taking matters into his/her own hands, for the better or for worse.The
ifnesting inDealloc()is unnecessary. The first condition can just be flipped and early return. The second can also be simplified from an if-then-else to an if-then-return.std::coutis not meant for error logging. You can however usestd::cerrif you want printed error output.cerr, orSTDERR, is the standard C++ stream for error output. But you might also consider replacing the error logs by exceptions, to give more flexibility to users of your code.Why is
InitializeHeaders()protected? You're not planning to inherit from this class, judging by the lack of avirtualdestructor, so anything internal should beprivate. By the way, this is a bit of a personal style, but I find it more relevant to place theprivatesection of a class right at the end. My rationale is that implementation details should not be the first thing you see when you look at a class declaration.Your decision of returning a
shared_ptrof the allocated block is questionable. What if all I need is aunique_ptr? My opinion is that this level of memory management is much too low to be dealing with smart pointers. All the user of a memory allocator/pool cares about is a pointer to the allocated block and the block size in bytes, so perhaps your allocator should deal in terms of aBlocktype? But I'm stealing this idea:P, I strongly suggest watching this excellent presentation: "std::allocator Is to Allocation what std::vector Is to Vexation".Memory alignment is most certainly going to be an issue with your allocator. The first object pulled from the memory pool will be aligned, because operator
newreturns properly aligned memory, but if the size of the allocated chunk is not evenly divisible by the minimum alignment, the second allocation won't be. This might result in misaligned memory access errors. You will very likely experience that if you try using your allocator with an__m128vector type that requires 16bytes alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_ofandstd::aligned_storage. You can also allow the specification of a minimum alignment as an integer template parameter.Tidy up those spurious blank lines here-and-there. One blank line is more than enough to separate two distinct areas of code.