Boost logo

Boost :

Subject: [boost] [align] [review] Pre-review comments
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2014-03-29 00:33:54


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_at_[hidden]>
Date: Thu Mar 27 17:45:12 2014 -0700



"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]


I notice that alignment is only required to be a
power of two. Is there a reason this is different
from align?


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.


    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<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

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.

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)

    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.

I seem to recall msvc warning that
memory is unused in cases like this.
You may need to supress the warning.


    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.


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++)


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.


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.


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


same as aligned_allocator_adaptor_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

Boost list run by bdawes at, gregod at, cpdaniel at, john at