Re: [boost] Boost.Align review begins today

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.