Boost logo

Boost :

Subject: Re: [boost] Boost.Align review begins today
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2014-04-12 09:45:33


On Friday 11 April 2014 20:48:02 Ahmed Charles wrote:
> Hi all,
>
> The review of Boost.Align by Glen Fernandes commences today, Friday, 11
> April, and concludes Sunday, 20 April.
>
> - What is your evaluation of the design?

Overall design looks mostly ok with some reservations below.

1. I'm not sure why some public symbols (align(), aligned_alloc(), etc) are
defined in the detail namespace. It would probably be better to move them to
the boost::alignment namespace. The reason I'm mentioning this is that the
detail namespace will come up in compiler errors and linkage symbols, which
might be inconvenint.
2. I think, it's not a good idea to import all Boost.Align symbols into
boost:: namespace. I could probably agree with importing align() and
aligned_alloc()/aligned_free() but not more sohpisticated things. Boost
namespace becomes more cluttered as it is. I'm open to discussion on this
topic.
3. It might be a good idea to provide separate headers with forward
declarations of aligned_allocator and aligned_allocator_adaptor. It's not a
mandatory requirement but I think the headers would be usefult in some cases.
4. aligned_allocator_adaptor relies heavily on constructing/destroying the
underlying allocator on every allocation/deallocation. This is fine in most
cases, except when: (a) the allocator is not stateless and the conversion to
CharAlloc actually takes time and (b) when the allocator constructor can throw
(this breaks deallocate()). I'm not sure how legitimate it would be to use
such an allocator with the adaptor but wouldn't it be better to just derive
from CharAlloc in the first place? You don't need the original Allocator
anyway. I know you followed scoped_allocator_adaptor design but I think its
use case is sufficiently different.

> - What is your evaluation of the implementation?

Mostly ok with the following notes:

1. is_alignment should be marked constexpr and noexcept. As well as
is_aligned.
2. I think, in many headers you're missing #include <cstddef> for std::size_t.
This at least concerns aligned_alloc*.hpp and align.hpp headers.
3. aligned_alloc_android.hpp and aligned_alloc_sunos.hpp are essentially the
same. Suggest leaving just a single aligned_alloc_memalign.hpp which can be
used on all platforms where the result of memalign() can be safely passed to
free().
4. aligned_alloc() and aligned_free() should be marked noexcept. align() could
also probably be marked, although the Standard does not define it as noexcept.
5. Empty constructors in aligned_allocator should probably be defaulted. You
can use BOOST_DEFAULTED_FUNCTION to achieve portability.
6. aligned_allocator.hpp and aligned_allocator_adaptor.hpp are missing
#include <new> for placement new and std::bad_alloc.
7. In aligned_allocator<T>::allocate(), why the hint is defined as
aligned_allocator<void>::const_pointer and not just const void*? You
unnecessarily instantiate aligned_allocator here.
8. aligned_allocator<T>::max_size() should be constexpr and noexcept.
address(), deallocate() and comparison opereators should be noexcept.
destroy() should be conditionally noexcept.
9. Suggest using BOOST_STATIC_ASSERT_MSG instead of BOOST_STATIC_ASSERT to
provide better diagnostics.
10. is_alignment and is_power_of_2 are used for the same purpose yet are named
differently. I think it'd be better to rename is_power_of_2 to something like
is_valid_alignment_static (while is_alignment could be is_valid_alignment).
There tools could be defined in a single header. Also, if constexpr is
supported is_valid_alignment_static can be implemented in terms of
is_valid_alignment.
11. In max_count_of.hpp, replace static_cast<std::size_t>(-1) with
~static_cast<std::size_t>(0).
12. In aligned_allocator_adaptor<T>::deallocate(), you attempt to
reinterpret_cast pointer to CharPtr*. This may fail, if pointer is not a raw
pointer type. Suggest to perform the cast in the more generic way, like you do
in allocate(): reinterpret_cast<CharPtr*>(detail::addressof(*memory)).
13. Feature request: Please, provide aligned_deleter function object in a
separate header. The function object should call aligned_free() to free memory
and can be used with unique_ptr. This pretty much is done in one of the
examples in the docs.

> - What is your evaluation of the documentation?

Rather poor:

1. Please, rewrite it in QuickBook. Not only it ensures the look and feel
common to other Boost libraries, it also adds other useful features, such as
Doxygen integration and importing the actual compilable code which is useful
for examples.
2. It might be worthwhile to describe the differences between C11 and
Boost.Align aligned_alloc() to avoid confision. Aside from aligned_free(),
Boost.Align aligned_alloc() has weaker requirements on the size argument.
3. I wonder if quoting the Standard violates any copyrights. I'm not a lawyer.
In any case, even from a learning user standpoint it might be better to use a
simpler language in the docs. A reference to the Standard (draft) could be
provided as well.
4. There is actually no documentation to review except for the reference
section. I know the library is small and most basic things are easy to grasp
for an educated programmer. But nevertheless, a few examples with description
wouldn't harm, especially involving allocators. FWIW, aligned_allocator and
aligned_allocator_adaptor are new things in C++ and should be properly
documented.
5. The Synopsis section does not represent any particular header of the
library. I'd rather see a separate synopsis sections for every header. In
fact, Doxygen provides mostly what is needed for the reference section.
6. The reference section should be placed near the end of the documentation.
The typical reading goes Introduction -> Tutorial -> Detailed description ->
Reference. Of course, your docs can omit some of these parts due to the
library simplicity, but the overall structure should be something like that.
Also, it's easier to read if the sections are of different pages.

All in all, most of the structural and aesthetical issues should be resolved
by moving to QuickBook. It shouldn't be difficult given the amount of the
hand-written docs. The reference could be auto-generated by Doxygen. What is
left is a proper description for the components and Doxygen comments.

> - What is your evaluation of the potential usefulness of the library?

Very useful. I've implemented similar functionality many times, it would be
useful to have it in one place.

> - Did you try to use the library? With what compiler? Did you have any
> problems?

I did not compile any code.

> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

A few hours reading the code and the docs.

> - Are you knowledgeable about the problem domain?

I believe so.

> Please always state in your review whether you think the library should be
accepted as a Boost library.

Yes, I think the library should be accepted on condition that the
documentation is improved. The current docs are insufficient.

I have outlined many other notes and suggestions in my review but none of them
render the library useless to be rejected. However, I'd like to kindly ask
Glen to address these issues.

Lastly, I'd like to thank Glen for this submission and Ahmed for managing the
review.


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk