Re: [boost] [align] Review - Impl Evaluation Part 1.

Subject: Re: [boost] [align] Review - Impl Evaluation Part 1.
From: Mostafa (mostafa_working_away_at_[hidden])
Date: 2014-04-12 05:21:02

2) What is your evaluation of the implementation?

#########################
align/detail/max_size.hpp
#########################

Is there a special reason to use this metafunction instead of
boost::static_unsigned_max, if so, then it sould be documented?

#############################
align/detail/max_count_of.hpp
#############################

Can "static_cast<std::size_t>(-1)" be replaced by
"boost::integer_traits<std::size_t>::const_max" ? std::size_t must be  
defined to be an
unsigned integral type which I think makes it a fundamental type, so it  
can be
used with std::numeric_limits, and, hence boost::integer_traits.

#############################
align/detail/is_alignment.hpp
#############################

This seems to be a runtime complement to the is_power_of_2 metafunction,  
if so,
then the latter should be renamed to reflect that. It's confusing to have
"is_alignment" and "is_power_of_2" which basically do the same thing,  
albeit one
at runtime and the other at compile time, yet which have completely  
different
names.

IMO, "is_alignment" and "is_power_of_2" should be merged into the same  
header,
the latter properly renamed, and the power-of-2 check folded into a macro  
that's
reused for both the function and metafunction (and remember to undef the  
macro
at the end since this is going to be included in a global header file).

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

"is_aligned", IMO, can be made more readable if it was renamed  
"is_aligned_to".
One reason for this is that the function "is_aligned" sounds too similar to
"is_alignment", so it causes some confusion as to what the actual  
functionality
of each is.

Additionally, "(address_t(ptr) & (alignment - 1))" is the fast algorithm  
for
calculating some number modulo a power of 2. I suggest replacing the body  
of the
currently named "is_aligned" function with:
   return modulo_alignment((address_t(ptr), alignment); //(*)
it makes the code more readable and self-documenting.

(*) Where modulo_alignment is defined in terms of:
   return modulo_power_of_2(value, alignment);

##############################
align/detail/aligned_alloc.hpp
##############################

//aligned_alloc:

IMO, n1 should be renamed p1_buf_size, and be const.

Since the result of "align(alignment, size, p1, n1);" is not being used,  
the
latter should be cast to void to suppress compiler warnings.

Should "alignment" be silently set to alignof(void *) if its more weak than
alignof(void *), or should the restrictions on "alignment" be a  
precondition for
calling this function and a BOOST_ASSERT check be made on it. Either way it
should be noted in the documentation. (More importantly, does this silent
behaviour also apply to the provided platform specific aligned_alloc  
functions?)

#########################################################
align/detail/aligned_alloc_{android,msvc,posix,sunos}.hpp
#########################################################

What happens when "size" parameter is 0? According to  
"boost::aligned_alloc"
specs it should return a null pointer. This also seems to be the behaviour  
of
aligned_alloc on both msvc and posix, but sunos maybe different (consult  
the
table below). On sunos there is a possibility that the current  
implementation of
aligned_alloc as found in aligned_alloc_sunos might return a non-null  
pointer,
in contravention of "boost::aligned_alloc" specs. If so the code needs to  
be
fixed.

I couldn't find any documentation on android's memalign, can you provide  
it or
link to it?

android: doc not found.
   (http://src.chromium.org/svn/trunk/src/base/memory/aligned_memory.cc) ?
   (http://code.google.com/p/android/issues/detail?id=35391) ?
msvc: http://msdn.microsoft.com/en-us/library/8z34s9c6.aspx
posix:  
http://pubs.opengroup.org/onlinepubs/007904975/functions/posix_memalign.html
sunos:
   http://docs.oracle.com/cd/E19253-01/816-5168/6mbb3hrgf/index.html
   http://docs.oracle.com/cd/E26502_01/html/E35299/mem-1.html
-------------------------|------------------------|-----------------------------
             msvc         |            posix       |            sunos
-------------------------|------------------------|-----------------------------
void * _aligned_malloc(  |int posix_memalign(     |void *memalign(
     size_t size,         |  void **memptr,        |  size_t alignment,
     size_t alignment);   |  size_t alignment,     |  size_t size);
                          |  size_t size);         |
-------------------------|------------------------|-----------------------------
alignment:               |alignment:              |alignment:
  must be an integer      | shall be a multiple of | must be a power of two  
and
  power of 2.             | sizeof( void *), that  | must be greater than or
Returns:                 | is also a power of two.| equal to the size of a  
word.
  A pointer to the memory |                        |
  block that was allocated|Returns:                |size:
  or NULL if the operation| 0 if sucessful, else !0| If 0, either a null  
pointer
  failed. The pointer is  |                        | or a unique pointer  
that can
  a multiple of alignment.|                        | be passed to free() is  
returned.

~~~~~
posix
~~~~~
Same issue with the handling of "alignment" as with
"align/detail/aligned_alloc.hpp".

######################
align/detail/align.hpp
######################

It's more clear if "address_t(ptr) & (alignment - 1)" was replaced by
"modulo_alignment((address_t(ptr), alignment)". In fact, I suggest the  
following
more clearly conveys your intent:
     std::size_t modulus_offset = 0;
     if (const std::size_t remainder =
             modulo_alignment((address_t(ptr), alignment) ) {
         modulus_offset = alignment - remainder;
     }
     // Onwards from here n1 in the original code is replaced by  
modulus_offset.
     ...

Should "align" BOOST_ASSERT if "alignment < alignment_of<void*>::value" ?  
Since,
per the doc, alignment shall be a fundamental alignment value or an  
extended
alignment value, and the alignment of "void *" represents the weakest  
alignment
requirement.

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

~~~~~~~~~
line 117:
~~~~~~~~~
The type of "value" looks like a universal reference to me, therefore
std::forward rather than std::move should be used with it.

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

In C++03 why not use boost::container::allocator_traits, which is a  
backport of
std::allocator_traits. (As an aside, should  
boost::container::allocator_traits
be factored out into its own library?)

Can you document why the following do not use their Traits counterpart:
     typedef value_type* pointer;
     typedef const value_type* const_pointer;
     typedef void* void_pointer;
     typedef const void* const_void_pointer;
     typedef std::ptrdiff_t difference_type;

~~~~~~~~
PtrAlign
~~~~~~~~

Please document why CharPtr and CharAlloc are needed, ie, that the adapted
allocator may return smart pointers, and not just regular pointers. It  
saves a
lot of guesswork and reverse engineering on part of future maintainers.

IMO, PtrAlign should be renamed CharPtrAlign. I would expect PtrAlign to  
be an
alias for "alignment_of<pointer>::value" and not  
"alignment_of<CharPtr>::value".

I found the name BlockAlign uninformative, its only purpose and intent  
seems to
be to serve as an intermediary place holder to calculate MaxAlign, maybe  
commenting
it would help? (At first glance I thought BlockAlign had something to with  
the
alignment of blocks of memory.)

Again, document the logic for MaxAlign, that it needs to take into account  
the
alignment of CharPtr because the latter is not necessarily an alias for  
"char *"
and that its alignment can be ">= alignof(char *)".

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pointer allocate(size_type size)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Please use descriptive variable names. Suggestions:
s/n1/requested_size
s/n2/padded_size
s/p1/pallocated_buf
s/p3/paligned_buf_header
and document that p2 starts out as "ppadded_buf" but is mutated to  
"paligned_buf".

The call to "align" should be cast to void to avoid spurious "return value  
not
used" compiler warnings.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pointer allocate(size_type size, const_void_pointer hint)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Same concerns as "pointer allocate(size_type size)".

Is the preprocessor conditional code really needed? Can't it all be  
replaced by
"a1.allocate(...)" since "a1" is required by the standard to define such a
function?

The logic for this function and "pointer allocate(size_type size)" can be
factored out into a templated function whose last parameter is a
template allocator functor. (Templated if you want to avoid an extra  
if-check,
else it could be non-templated.)

I don't see the point of "h1 = *(static_cast<const CharPtr*>(hint) - 1);".
Presumably the point of using this overloaded version of "allocate" is to
allocate memory at memory address "hint", not construct an object there,  
since
the starting hint address is not guaranteed to be aligned properly.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
void deallocate(pointer memory, size_type size)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I suggest renaming p2 to pallocated_buf.