Subject: Re: [boost] Boost.Align review begins today
From: Glen Fernandes (glen.fernandes_at_[hidden])
Date: 2014-04-21 02:09:42
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_traits<std::size_t>::const_max"
> instead of the less readable "~static_cast<std::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