[align] Review.

older
[algorithm] any_of does not call...

Mostafa

12 Apr 2014 12 Apr '14

9:11 a.m.

For easier tracking by the Review Manager I am going to break up my review into multiple parts which will be top-level responses to this post.

Show replies by date

Mostafa

12 Apr 12 Apr

9:14 a.m.

New subject: [align] Review - Design Evaluation.

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 struct aligned_allocator; template struct uniform_alignment_traits { template <typename T> struct alignment_of { static std::size_t const value = Alignment; }; }; template struct aligned_allocator : public aligned_allocator { }; 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 >" 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 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 struct allocator_adaptor; that defaults to std::allocator_traits 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".

Glen Fernandes

9:28 a.m.

New subject: [align] Review - Design Evaluation.

On Sat, Apr 12, 2014 at 2:14 AM, Mostafa wrote: Thank you! Some context regarding two concerns regarding design:

...

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:

This design was chosen because it is the convention for allocator adaptors, introduced into the C++ standard library in C++11 with scoped_allocator_adaptor. (template class scoped_allocator_adaptor : public OuterAlloc ...)

...

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.

Similarly, chosen for consistency with the design established with scoped_allocator_adaptor. (template <class OuterA2> scoped_allocator_adaptor(OuterA2&& outerAlloc, const InnerAllocs&... innerAllocs) ...) Glen

Mostafa

9:21 a.m.

New subject: [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.

Marc Glisse

9:46 a.m.

New subject: [align] Review - Impl Evaluation Part 1.

On Sat, 12 Apr 2014, Mostafa wrote:

...

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

http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_memalign.htm... says that for size 0, the return may be either null or a unique pointer (since 2008). C11 says the same about the aligned_alloc function. Old freebsd used to assert that the size argument was != 0. -- Marc Glisse

Glen Fernandes

9:58 a.m.

New subject: [align] Review - Impl Evaluation Part 1.

On Sat, Apr 12, 2014 at 2:21 AM, Mostafa wrote:

...

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

The intention is that aligned_alloc() should have the same requirements whether it uses my implementation, MSVC's, POSIX's, or any other. So if a particular implementation specific version has stronger requirements for the value of 'alignment', my inline aligned_alloc wrapper around that particular platform-specific function will relax those requirements (by rounding up 'alignment' if necessary).

...

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

The documentation should be written more clearly to reflect this; the intention is that if zero is passed, then the value returned could be a null pointer, or it could be non-null, but use of that pointer is undefined. It can only be passed to aligned_free() and nothing else.

...

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.

An alignment smaller than alignof(void*) is still a valid fundamental alignment, so align() should be usable with such values.

...

pointer allocate(size_type size, const_void_pointer hint) 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?

While [allocator.requirements] implies that every allocator should provide an overload for allocate() that takes hint, [allocator.traits.members] 20.6.8.2/2 implies that for a given allocator, allocate(n, hint) may not be well-formed, and so allocator_traits<...>::allocate(n, hint) will call allocate(n) if allocate(n, hint) is not well-formed.

...

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.

The inner allocator's allocate(..., hint) can assume that hint is a value previously returned by its allocate() (on an equivalent allocator object). The hint passed to the adaptor would not be that value - it would be the value returned by a call to allocate() on the adaptor. So: This just ensures that the hint passed to the inner allocator is the right value. Glen

Mostafa

8:11 p.m.

New subject: [align] Review - Impl Evaluation Part 1.

On Sat, 12 Apr 2014 02:58:48 -0700, Glen Fernandes wrote:

...

On Sat, Apr 12, 2014 at 2:21 AM, Mostafa wrote:

...

...
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?) The intention is that aligned_alloc() should have the same requirements whether it uses my implementation, MSVC's, POSIX's, or any other. So if a particular implementation specific version has stronger requirements for the value of 'alignment', my inline aligned_alloc wrapper around that particular platform-specific function will relax those requirements (by rounding up 'alignment' if necessary).

Unfortunately I don't understand what you're trying to say. Additionally let me rephrase my question, why is it that in the absence of a platform-specific aligned_alloc the boost provided aligned_alloc silently aligns on alignof(void *) if the user requested alignment value is less than alignof(void *)? (I understand this is still correct behaviour, just curious since the boost provided aligned_alloc uses boost::align to align the memory buffer and, per your previous response, "An alignment smaller than alignof(void*) is still a valid fundamental alignment, so align() should be usable with such values.")

...

...
pointer allocate(size_type size, const_void_pointer hint) 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?

While [allocator.requirements] implies that every allocator should provide an overload for allocate() that takes hint, [allocator.traits.members] 20.6.8.2/2 implies that for a given allocator, allocate(n, hint) may not be well-formed, and so allocator_traits<...>::allocate(n, hint) will call allocate(n) if allocate(n, hint) is not well-formed.

Actually it doesn't imply that, and I was wrong in my original assertion, 17.6.3.5/2: "Table 28 describes the requirements on allocator types and thus on types used to instantiate allocator_traits. A requirement is optional if the last column of Table 28 speciﬁes a default for a given expression." and "a.allocate(n, u)" defaults to "a.allocate(n)" in the said table.

Glen Fernandes

8:31 p.m.

New subject: [align] Review - Impl Evaluation Part 1.

On Sat, Apr 12, 2014 at 1:11 PM, Mostafa wrote:

...

Unfortunately I don't understand what you're trying to say. Additionally let me rephrase my question, why is it that in the absence of a platform-specific aligned_alloc the boost provided aligned_alloc silently aligns on alignof(void *) if the user requested alignment value is less than alignof(void *)? (I understand this is still correct behaviour, just curious since the boost provided aligned_alloc uses boost::align to align the memory buffer and, per your previous response, "An alignment smaller than alignof(void*) is still a valid fundamental alignment, so align() should be usable with such values.")

Ah, I had misinterpreted your question. The reason why the Boost-provided aligned_alloc will ensure alignment is a minimum of alignof(void*), is because after calling align(), we want to access the sizeof(void*) bytes immediately preceding the aligned memory as a void*, so that address must be suitably aligned for void*. Instead of *(static_cast(p1) - 1) = p2; I could have used std::memcpy to copy p2 into that storage, and then it would not require that storage to be at-least alignof(void*) aligned.

...

Actually it doesn't imply that, and I was wrong in my original assertion, 17.6.3.5/2:

"Table 28 describes the requirements on allocator types and thus on types used to instantiate allocator_traits. A requirement is optional if the last column of Table 28 speciﬁes a default for a given expression."

and "a.allocate(n, u)" defaults to "a.allocate(n)" in the said table.

*Nod*. Alternatively one could say that maybe the adaptor should only provide an allocate(n, hint) overload if the inner allocator fit provides an allocate(n, u) overload, but I prefer this design. Glen

Mostafa

15 Apr 15 Apr

11:19 p.m.

New subject: [align] Review - Impl Evaluation Part 1.

Glen Fernandes writes:

...

On Sat, Apr 12, 2014 at 1:11 PM, Mostafa

...

...
Unfortunately I don't understand what you're trying to say. Additionally let me rephrase my question, why is it that in the absence of a platform-specific aligned_alloc the boost provided aligned_alloc silently aligns on alignof(void *) if the user requested alignment value is less

yahoo.com> wrote: than

...

...
alignof(void *)? (I understand this is still correct behaviour, just curious since the boost provided aligned_alloc uses boost::align to align the memory buffer and, per your previous response, "An alignment smaller than alignof(void*) is still a valid fundamental alignment, so align() should be usable with such values.")

Ah, I had misinterpreted your question. The reason why the Boost-provided aligned_alloc will ensure alignment is a minimum of alignof(void*), is because after calling align(), we want to access the sizeof(void*) bytes immediately preceding the aligned memory as a void*, so that address must be suitably aligned for void*.

Instead of *(static_cast(p1) - 1) = p2; I could have used std::memcpy to copy p2 into that storage, and then it would not require that storage to be at-least alignof(void*) aligned.

Thanks for the explanation. Then I think the documentation for aligned_alloc needs to be updated from: "... by an additional sizeof(void*) and alignment bytes." to: "... by an additional sizeof(void*) and max(alignment, sizeof(void *)) bytes." to take into account the silent alignment bumping that may occur in some scenarios.

Mostafa

12 Apr 12 Apr

9:22 a.m.

New subject: [align] Review - Documentation Evaluation Part 1.

3) What is your evaluation of the documentation? ##################### Introduction section: ##################### The sentences in this section should all be folded into one paragraph. -- -- s/It provides allocation functions aligned_alloc and aligned_free as/ It provides the functions aligned_alloc and aligned_free as Rationale: aligned_free is not an allocation function. -- -- s/It provides C++ allocators, class templates aligned_allocator and aligned_allocator_adaptor, which respect alignment./It provides C++ allocators aligned_allocator and aligned_allocator_adaptor which respect alignment. s/The first uses aligned_alloc and aligned_free while the second uses the allocator in conjunction with align./The first uses aligned_alloc and aligned_free while the second uses a user specified allocator in conjunction with align. ################### Vocabulary section: ################### Can you indicate that this cites the C++11 standard? ######################## Interface documentation: ######################## The documentation of function semantics is inconsistent, at times it uses C++ Standards' notation, at other times it uses C Standards' notation. IMO, it should be normalized on C++ Standards' convention since Boost specifically targets C++ libraries. ###################### aligned_alloc section: ###################### Replace "Description" subsection with: Requires: The value of alignment shall be a power of two. Effects: The aligned_alloc function allocates space for an object whose alignment is specified by alignment, whose size is specified by size, and whose value is indeterminate. ##################### aligned_free section: ##################### Replace "Description" subsection with: Requires: ptr must be a value returned by aligned_alloc or a null pointer, and it must not have been used as parameter to some earlier call of aligned_free. Effects: The aligned_free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs. ########################## aligned_allocator section: ########################## There are overloads of aligned_allocator::construct in the header file which are not accounted for in the documentation. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "aligned_allocator notes" section: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This section should be moved under "aligned_allocator requirements", else it's easy to miss it. ######################### aligned_allocator_adaptor ########################## ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ aligned_allocator_adaptor requirements section: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The requirements on template paramter Allocator in C++03 should be documented, ie, it needs to define "rebind", "pointer", "size_type", since std::allocator_traits is absent from that standard, unless its decided that boost::container::allocator_traits will be used. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ aligned_allocator_adaptor constructors section: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This section is missing the following C++03 variant: "explicit aligned_allocator_adaptor(const A& alloc)" ~~~~~~~~~~~~~~~~~~~~~~~~~~~ aligned_allocator_adaptor() ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Missing: Requires: Allocator type be default constructible. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pointer allocate(size_type n) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The following replacement for "Remarks" section is IMO much simpler and clearer: Remarks: The storage is obtained by calling A2::allocate on an object a2, where A2 is of type Allocate::rebind<some-unspecified-type>::other. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pointer allocate(size_type n, const_void_pointer hint) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Same issues as "pointer allocate(size_type n)". Does the "Requires" section mean: aligned_allocator_adaptor<some-unspecified-type> a1; auto hint = a1.allocate(1); aligned_allocator_adaptor<some-type> a2; auto obj = a2.allocate(some-value, hint); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ void deallocate(pointer p, size_type n) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Same issues as "pointer allocate(size_type n)". ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ aligned_allocator_adaptor notes section: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This section should be moved under "aligned_allocator_adaptor requirements", else it's easy to miss it. And can you document why (the Rationale) this adaptor can only return raw pointers. ################### is_aligned section: ################### Replace "Description" subsection with: Requires: alignment is a power of two. ptr is a valid address.

Mostafa

9:23 a.m.

New subject: [align] Review - Documentation Evaluation Part 2.

3) What is your evaluation of the documentation? #################### Format and Structure #################### One problem I have with the current documentation is that it's overly long. Scrolling down, I easily loose track of which section I'm in or I easily scroll past the (small) section I wanted to go to next. I recommend structuring the documentation like how Boost.Move and Boost.Container are structured. It makes navigation and digesting the information much easier.

4033

Age (days ago)

4036

Last active (days ago)

List overview

Download

10 comments

3 participants

participants (3)

Glen Fernandes
Marc Glisse
Mostafa

[align] Review.

Mostafa

Mostafa

Glen Fernandes

Mostafa

Marc Glisse

Glen Fernandes

Mostafa

Glen Fernandes

Mostafa

Mostafa

Mostafa

tags

participants (3)