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.

2014-04-12 7:48 GMT+04:00 Ahmed Charles : <...>

The review of Boost.Align by Glen Fernandes commences today, Friday, 11 April, and concludes Sunday, 20 April.

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

Library must be included into the Boost, after minor improvements.

- What is your evaluation of the design?

Good.

- What is your evaluation of the implementation?

Andrey Semashev already pointed out most of the issues. I'd like to add the following: 1) Looks like include/boost/align/detail/aligned_alloc.hpp in worst case uses more memory than required // Looks like we can extract 1 from n1, // because boost::align in worst case will add // (alignment - 1) bytes. std::size_t n1 = size + alignment; 2) BOOST_NOEXCEPT is missing all around the library. This is essential for copy constructors and gives user more info about method's behavior. 3) Feature request: aligned_allocator_adaptor won't work with allocators from Boost.Interprocess. I'd love to this this resolved. This may require additional boost::alignment::align overload for pointer types used in Boost.Interprocess and a few minor fixes to aligned_allocator_adaptor. 4) There's no tests for aligned_allocator_adaptor with stateful allocators. Please add some more tests. 5) Use boost::throw_exception(e) instead of throw e. This will make Align library usable even with exceptions disabled.

- What is your evaluation of the documentation?

OK. With help of Paul A. Bristow docs will become perfect. - What is your evaluation of the potential usefulness of the library?

Useful. Was missing such functionality a few weeks ago.

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

No, just looked through the sources and docs.

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

Spend more than two hours reading the source codes and docs.

- Are you knowledgeable about the problem domain?

I do know the theory and used a self-made aligned_alloc a few times. -- Best regards, Antony Polukhin

On Fri, Apr 11, 2014 at 08:48:02PM -0700, Ahmed Charles wrote:

The review of Boost.Align by Glen Fernandes commences today, Friday, 11 April, and concludes Sunday, 20 April. Please always state in your review whether you think the library should be accepted as a Boost library.

I believe that the Align library should be accepted into Boost without any conditional requirements from my side.

Additionally, please consider giving feedback on the following general topics:

- What is your evaluation of the design?

The aligned allocation/destruction functions are similiar in shape to the platform specific functions with similiar names. Judging by the documentation, the allocators and functions have as natural interfaces as possible, which is good.

- What is your evaluation of the implementation?

I've skimmed the implementation and no apparent madness stands out.

- What is your evaluation of the documentation?

Documentation as it currently stands needs some Boostification, but I believe that has been addressed already. I find the standardese-like reference readable.

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

This library is very useful to anyone that ever has needed to deal with alignment, saving them from adapting and reinventing the facilities present in the library.

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

I ran the bundled tests with a set of compilers with no additional flags, with Align implanted into a modular Boost checkout. Works perfectly: GCC 4.6.3, Intel 13.0, Intel 13.1 Compiler errors: PGI 13.10, PGI 14.3 (needed to modify Boost.Build to avoid linker failures) A lot of ambiguities like "aligned_allocator_adaptor_test.cpp", line 31: error: "allocator" is ambiguous Intel 12.0 Breaks in Boost.Config on __int128, so not really Align's fault. Runtime errors: PSC 5, may be due to site customizations of the toolchain so may be disregarded, I have to look more into it. is_aligned_test.cpp fails intermittently for sizes 128, 64, and sometimes 32. is_aligned_test.cpp(44): test 'boost::alignment::is_aligned(32, &s[0])' failed in function 'int main()' is_aligned_test.cpp(49): test 'boost::alignment::is_aligned(64, &s[0])' failed in function 'int main()' is_aligned_test.cpp(54): test 'boost::alignment::is_aligned(128, &s[0])' failed in function 'int main()' Fails 'align_test.cpp' assertions on lines 23, 25, 31, 32, 33, 40, 42; once for 64 and once for 128 bytes. Sadly SunStudio and VisualAge are out of my reach at the moment, checking out modular Boost on those filesystems would take days. I can provide logs on request.

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

An evening skimming and reflecting on the documentation and intended semantics, a morning running the test suites on any compilers I could reach.

- Are you knowledgeable about the problem domain?

I've reinvented most facilities of this library in more or less horrible ways in the past. -- Lars Viklund | zao@acc.umu.se

On Fri, Apr 11, 2014 at 8:48 PM, Ahmed Charles wrote:

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

This library should be accepted as a Boost library.

- What is your evaluation of the design?

Solid and follows existing practices.

- What is your evaluation of the implementation?

The implementation is good and clean. There is one issue in that detecting the alignment of a pointer is done using implementation defined behaviour. But since every compiler implements this the same way, I think it's acceptable.

- What is your evaluation of the documentation?

The documentation is very readable and complete.

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

I see quite a few use cases for this library and that it adds some much needed functionality that has yet to make it into the standard.

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

No.

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

Spent about an hour reading the code and docs.

- Are you knowledgeable about the problem domain?

I'm quite knowledgeable about alignment in C++. I presented at C++Now about the changes to alignment in C++11 and the remaining problems we need to fix. - Michael Spencer

On 04/12/2014 05:48 AM, Ahmed Charles wrote:

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

This library should be unconditionally accepted.

- What is your evaluation of the design?

Good design. Good compliance/integration with std.

- What is your evaluation of the implementation?

Good quality. Use Boost.Predef to check for pre-defined macros.

- What is your evaluation of the documentation?

The documentation of the examples is too meager. Please add a description of the purpose of each example, so that people can assess if the example is relevant to them before they start deciphering the code.

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

Alignment is a recurring task, so the library is useful.

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

No.

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

I have spent a couple of hours in total looking at the code and the documentation.

- Are you knowledgeable about the problem domain?

Adequately.

On Fri, 11 Apr 2014 20:48:02 -0700, Ahmed Charles wrote: (Ahmed, a piece of advice, the review period doesn't end until the end of the review date for all time zones.)

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

No. The author has unfortunately not responded to a majority of questions and concerns I had brought up. (Note: this is solely about a lack of response, and not any disagreements on the direction of library which I may have with the author.) Some of the questions and concerns were quite trivial and non-controversial, such as: 1. Documentation. 1.1) The documentation uses both C and C++ conventions in documenting function semantics, why not normalize on C++ conventions? 1.2) Why isn't "is_aligned" documented to note that a precondition on its "alignment" parameter is that it should be a power of 2? 1.3) There are overloads of aligned_allocator::construct in the header file which are not accounted for in the documentation. 1.4) The following C++03 variant is missing from the documentation: "explicit aligned_allocator_adaptor(const A& alloc)" 1.5) Why not document the reason why even though aligned_allocator_adaptor can be used with smart pointers it will only expose raw pointers? 2. Design 2.1) "aligned_allocator_adaptor::base_allocator()" reads more intuitively than "aligned_allocator_adaptor::allocator()". The latter forces me ask "which allocator?". 3. Implementation 3.1) Why not reuse boost::static_unsigned_max in max_align? 3.2) Why not the more readable "boost::integer_traitsstd::size_t::const_max" instead of the less readable "~static_caststd::size_t(0)"? 3.3) is_alignment and is_alignment_const duplicate logic, can't they be merged into the same header and share that logic via a macro? 3.4) When not capturing the return values of functions, they should be explicitly disgarded by casting the function call to void so as to avoid compiler warnings. (This pertains to calls to detail::align.) 3.5) The variable "value" is a universal reference in "void aligned_allocator::construct(U* memory, U&& value)" and should not be moved, rather it should be forwarded. 3.6) aligned_allocator_adaptor::allocate overloads share a lot of functionality, why can't that functionality be factored out into a common function? 3.7) There are some functions that have three or four 1 and 2 letter variable names. I find this unprofessional. These variables can easily be given descriptive names. My biggest concern is item 3.5, unless I'm wrong, that's a silent and dangerous bug waiting to happen. I'm not going to bring up any radical ideas I put forward, but I will bring up the issue I had with aligned_allocator_adaptor deriving from its adapted-to-type. For reasons noted earlier I think this is a mistake, and the feedback from the author that the standard scoped_allocator_adaptor does a similar thing still does not justify this design decision IMO.

What is your evaluation of the design?

I don't have any real issues with it except for what I have stated previously in this message.

What is your evaluation of the implementation?

Overall it does what it set out to do. But as noted previously in this message, there are a plethora of 1 and 2 letter variables names in some functions that can easily be replaced by more descriptive names, there is code duplication where there doesn't conceptually need to be any, and I think there is a dangerous bug lurking in the code.

What is your evaluation of the documentation?

As noted in previous messages I think it can be better formatted and structured. As noted in previously in this message I think there are omissions and think it should be normalized on C++ documentation conventions.

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

I never personally had a use for its functionality, therefore I don't feel qualified to give an answer.

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

No.

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

An in-depth study of the implementation and documentation. I wanted to but did not get around to reviewing the accompanying tests. Though, one thing I wanted to point out is that there should be a smart pointer test for aligned_allocator_adaptor since it does claim to work with smart pointer allocators.

Are you knowledgeable about the problem domain?

I know just enough to review the library.