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
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 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& 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_caststd::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
