Re: [boost] Boost.Align review begins today

Subject: Re: [boost] Boost.Align review begins today
From: Ahmed Charles (acharles_at_[hidden])
Date: 2014-04-21 02:15:02

----------------------------------------
> To: boost_at_[hidden]
> From: mostafa_working_away_at_[hidden]
> Date: Sun, 20 Apr 2014 21:46:34 -0700
> Subject: Re: [boost] Boost.Align review begins today
>
> 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.)

Sorry for believing it was GMT/UTC based.

>> 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.

Thanks for your review. I can send out an update with any additional conditions we can agree on, since the result is still conditional acceptance even with your vote, if you would like. Unfortunately, your initial review didn't clearly state a vote, so it wasn't included in the initial summary.

> (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.)

I believe the spirit of the review process is to mostly review the code and not 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?

These points seem reasonable to address with the work that will happen on the documentation.

> 2. Design
> 2.1) "aligned_allocator_adaptor::base_allocator()" reads more intuitively
> than "aligned_allocator_adaptor::allocator()". The latter forces me ask
> "which allocator?".

I'd agree with this suggestion.

> 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)"?

I think I agree with the sentiment that these are simple enough use cases that additional dependencies aren't required. Though, it seems that there is already a dependency on Boost.Integer, so that makes me more on the fence.

> 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.

I talked to Glen about this and proposed a solution that doesn't have this issue.

> 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.
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost