[boost] [align] [review] Pre-review comments

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

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_at_[hidden]>
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