Boost logo

Boost :

Subject: Re: [boost] Boost.Align review begins today
From: Mostafa (mostafa_working_away_at_[hidden])
Date: 2014-04-21 00:46:34

On Fri, 11 Apr 2014 20:48:02 -0700, Ahmed Charles <acharles_at_[hidden]>

(Ahmed, a piece of advice, the review period doesn't end until the end of
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
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

Some of the questions and concerns were quite trivial and
such as:

1. Documentation.
   1.1) The documentation uses both C and C++ conventions in documenting
     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
     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
     instead of the less readable "~static_cast<std::size_t>(0)"?
   3.3) is_alignment and is_alignment_const duplicate logic, can't they be
     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
     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
   3.7) There are some functions that have three or four 1 and 2 letter
     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
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
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
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
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
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
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?


> 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
not get around to reviewing the accompanying tests. Though, one thing I
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

> Are you knowledgeable about the problem domain?

I know just enough to review the library.

Boost list run by bdawes at, gregod at, cpdaniel at, john at