Re: [boost] Boost.Align review begins today

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