Re: [boost] [align] Review - Design Evaluation.

Subject: Re: [boost] [align] Review - Design Evaluation.
From: Mostafa (mostafa_working_away_at_[hidden])
Date: 2014-04-12 05:14:09

1) What is your evaluation of the design?

###########################
align/aligned_allocator.hpp
###########################

What about generalizing the interface of aligned_allocator to allow  
type-specific
alignment values? I know this was discussed on the mailing list and there  
was
some concern about interface complexity, that is having to define a  
metafunction
for the degenerate but common case where all types should be aligned  
equally.
I think that concern can be addressed and still allow interface  
flexibility with
something along the lines of the following:

     template <typename T, typename AlignmentTraits> struct  
aligned_allocator;

     template <std::size_t Alignment>
     struct uniform_alignment_traits
     {
       template <typename T>
       struct alignment_of
       {
         static std::size_t const value = Alignment;
       };
     };

     template <typename T, std::size_t Alignment>
     struct aligned_allocator<T, boost::mpl::size_t<Alignment> >
       : public aligned_allocator<T, uniform_alignment_traits<Alignment> >
     {
     };

where the second parameter can be an mpl::size_t type or a type with a  
nested
single argument template class named "alignment_of" that has a static  
member
variable named value. This allows for the degenerate case to be easily  
declared
as: "aligned_allocator<T, mpl::size_t<64> >" and yet still preserve the  
option
of specifying per-type alignment requirements.

####################################
align/aligned_allocator_adaptor.hpp
####################################

The same remarks about generalizing the interface as in
"align/aligned_allocator.hpp".

IMO, "class aligned_allocator_adaptor : public Allocator" is a bad design.
One, conceptually aligned_allocator_adaptor is not an "Allocator" because  
it
overrides some of "Allocator"'s core functionality, and two, it enables the
following surprising behaviour:
    aligned_allocator_adaptor<some_existing_allocator> aaa;
    some_existing_allocator & sea = aaa;

    aaa.allocate(...) != sea.allocate(...)
    aaa.deallocate(...) != sea.deallocate(...)
Inheritance should be private with the appropiate adapted-to-allocator's  
traits
being brought in with using statements or adapted-to-allocator should be a
member variable of aligned_allocator_adaptor.

As an aside, a general allocator_adaptor class would be useful, something  
along
the lines of the following:
     template <tyepname DERIVED, typename BASE_ALLOCATOR>
     struct allocator_adaptor;
that defaults to std::allocator_traits<BASE_ALLOCATOR> for the standard  
required
allocator type members not defined in DERIVED.

~~~~~~~~~~~~~~~~~~~
... allocator() ...
~~~~~~~~~~~~~~~~~~~

I suggest renaming the member function "allocator()" to  
"base_allocator()". One
too many times I thought it referred to the allocator adaptor itself.

~~~~~
ctors
~~~~~

Why isn't a general forward-to-base-allocator constructor provided?

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
template<class A> explicit aligned_allocator_adaptor(A&& alloc)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Is this function intended to forward *any* single argument to the base  
class
constructor? If so, then the argument name "alloc" is misleading and  
should be
changed. If not, then is the intent to forward some base class allocator of
arbitrary type, like the following:

   template <class A>
   explicit aligned_allocator_adaptor(
     typename Traits:: template rebind<A>::other && alloc)

   template <class A>
   explicit aligned_allocator_adaptor(
     typename Traits:: template rebind<A>::other & alloc)

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
template <class A> explicit aligned_allocator_adaptor(const A& alloc)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Same question as its C++11 counterpart.

####################
align/is_aligned.hpp
####################

As noted elsewhere, I think the name "is_aligned_to" more clear expresses  
this
function's intent than "is_aligned".