Re: [boost] Boost.Align review begins today

----------------------------------------

To: boost@lists.boost.org From: mostafa_working_away@yahoo.com 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 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_traitsstd::size_t::const_max" instead of the less readable "~static_caststd::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