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

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_caststd::size_t(-1)" be replaced by "boost::integer_traitsstd::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::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(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.