AMDG I saw the announcement of the review and decided to take a look at the library today. All the following is based on: commit 0d361143c18254efddacfd48775a2776ca7d5e11 Author: Glen Fernandes <glenfe@live.com> Date: Thu Mar 27 17:45:12 2014 -0700 Documentation: align: "alignment shall be a fundamental alignment value or an extended alignment value and a power of two." Please define these terms. Also warning: suggest parentheses around ‘and’ within ‘or’ [-Wparentheses] aligned_alloc: I notice that alignment is only required to be a power of two. Is there a reason this is different from align? aligned_allocator: template<class T, size_t Alignment = 1> class aligned_allocator; I'd really like to see the requirements on Alignment written out somewhere, even if we can deduce it from the fact that it calls aligned_alloc. Also, does the standard guarantee that all alignments are powers of 2? Otherwise lcm(2^k, alignof(T)) is not guaranteed to be a power of 2. If it is guaranteed, then lcm(2^k, alignof(T)) == max(2^k, alignof(T)) This is probably worth noting. aligned_allocator notes: "Specifying minimum alignment is generally only suitable for containers such as vector and undesirable with other, node-based, containers." I had to think about this for a few seconds. If you're going to say this, you should probably explain why. aligned_allocator_adaptor: template<class A> aligned_allocator_adaptor(A&& alloc); Not explicit? Should aligned_allocator_adaptor really be convertible from anything? template<class U> aligned_allocator_adaptor(const aligned_allocator_adaptor<U, Alignment>& other); Requires: Allocator shall be constructible from A. Copy/paste error. There is no A here. pointer allocate(size_type n, const_void_pointer hint = 0); Requires: hint is a value obtained by calling allocate(), or else nullptr. Calling allocate on what? Not just this object, but any equivalent aligned_allocator_adaptor, should work. aligned_allocator_adaptor notes "This adaptor class can be used with any C++11 or C++03 allocator including those allocators whose pointer type is a smart pointer." I don't see how this could possibly work with a smart pointer when aligned_allocator_adaptor explicitly uses raw pointers. Your tests pass for we with msvc 9.0-12.0, gcc-mingw-4.6.2, and clang-3.4 on Windows and with gcc 4.7.2 on Linux. aligned_allocator.hpp:100 aligned_free(memory); This is vulnerable to ADL if std::aligned_free is ever created (or my_ns::aligned_free for that matter). (Note that the corresponding aligned_alloc is safe becase std::size_t has no associated namespaces) aligned_allocator.hpp:104 enum { Size = static_cast<std::size_t>(-1) / sizeof(T) }; There's absolutely no reason to use an enum as far as I can see. You can just return this value. aligned_allocator.hpp:139 memory->~U(); I seem to recall msvc warning that memory is unused in cases like this. You may need to supress the warning. detail/align.hpp:28 if (n1 <= space && size <= space - n1) You might add a comment about avoiding overflow, so that no one is tempted to refactor it into an simpler "equivalent" test. test/Jamfile.v2: This is a lot more verbose than it needs to be. Try just: import testing ; run align_test.cpp ; run aligned_alloc_test.cpp ; ... test-suite is obsolete (it just groups all the targets under a single name, which is completely pointless when that's all you have in the Jamfile) and the braces just create a compound statement (like in C++) test/align_test.cpp: You need to test the following: - There is not enough space in the buffer. - In particular, you need to validate the exact boundaries where align starts to return 0. test/aligned_alloc_test.cpp: You need tests for the following conditions: - size == 0 - size < alignment - size > alignment && gcd(size, alignment) != alignment - the allocation fails because size is too large. - Also write to the allocated memory to (hopefully) verify that the data structure used by aligned_alloc is safely outside this region. test/aligned_allocator_adaptor_test.cpp: You're only testing allocate/deallocate. You also need tests for the other members. - rebind - copy constructor/copy assignment operator - construct/destroy - allocate with a hint - allocate with 0 size test/aligned_allocator_test.cpp: same as aligned_allocator_adaptor_test.cpp. test/is_aligned_test.cpp You need tests for: - is_aligned returns false. Your entire test suite would pass with bool is_aligned(...) { return true; } In Christ, Steven Watanabe