Hi Mostafa, My apologies for the lack of response; I was anticipating a conclusion to your review (in the form of the response to those five questions and a vote), not realizing that you were awaiting a reply from me first. As always, thank you for providing feedback. On Sun, Apr 20, 2014 at 9:46 PM, Mostafa wrote:
1. Documentation.
All the feedback about documentation seems reasonable, and I will address it when the documentation is rewritten.
2.1) "aligned_allocator_adaptor::base_allocator()" reads more intuitively than "aligned_allocator_adaptor::allocator()". The latter forces me ask "which allocator?".
Just a preference for the identifiers "allocator" or "inner_allocator" there, but the latter may lead people to think about the scoped_allocator_adaptor interpretation of inner/outer. I'll consider renaming it to "base_allocator".
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)"?
Both were to avoid a dependency on Boost.Integer for what are trivial detail types. The library design has preferred minimizing those dependencies in general (e.g. it depends on boost::addressof only if a conforming std::addressof is not available).
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?
Not entirely happy with the idea of using a macro to reduce that duplication (at least not for these trivial detail helpers).
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.)
While I haven't used that in the other Boost library I've contributed to (nor seen it used much in other Boost code), I'm not opposed to it.
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.
The construct overloads were changed (a few hours ago).
3.6) aligned_allocator_adaptor::allocate overloads share a lot of functionality, why can't that functionality be factored out into a common function?
Sure; this is something I think I can look into doing. Glen