Re: [boost] Boost.Align review begins today

On Sat, Apr 12, 2014 at 6:45 AM, Andrey Semashev wrote:

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.

Again, thank you for the help, Andrey. I've started on documentation improvements (including migration to QuickBook) today. On Monday I'll use the Develop branch to start addressing some of the review comments, while leaving the Master branch untouched to not disrupt anyone else reviewing it. Glen On Sat, Apr 12, 2014 at 6:45 AM, Andrey Semashev wrote:

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_caststd::size_t(-1) with ~static_caststd::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(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.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost