Re: [boost] Boost.Align review begins today

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]>  
wrote:

(Ahmed, a piece of advice, the review period doesn't end until the end of  
the
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  
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_traits<std::size_t>::const_max"
     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  
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.