On Fri, 11 Apr 2014 20:48:02 -0700, Ahmed Charles
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.