storage_ should be aligned for impl_type

[glenfe]

For new(storage_) impl_type(std::forward<arg_types>(args)...) to be well-defined behavior, storage_ needs to be aligned for impl_type.

This should be sufficient:

alignas(impl_type) char storage_[32];

[yet-another-user]

Thanks, Glen. It is embarrassing (for me) that you found something so quickly... and so basic... after you pointed it out. :-) Many thanks.

[glenfe]

Instead of:

using storage_type = boost::aligned_storage<sizeof(size_type), sizeof(size_type)>;

I suggest: using storage_type = boost::aligned_storage<sizeof(size_type), boost::alignment_of::value>::type;

In an example where impl_type has, say, 16 byte alignment and 1600 byte size.

[yet-another-user]

Yes, Glen, I tried applying the alignment as you indicated way before when you just opened the ticket. Now I tried again and it fails for the same reason:

/home/vbatov/dev/boost/boost/type_traits/alignment_of.hpp:82:39: error: invalid application of ‘alignof’ to incomplete type ‘impl_ptr::implementation’

That's because "implementation" is not visible (opaque) in the header... I think. :-)

On Mon, Aug 21, 2017 at 12:39 AM, Glen Fernandes notifications@github.com wrote:

Instead of:

using storage_type = boost::aligned_storage<sizeof(size_type), sizeof(size_type)>;

I suggest: using storage_type = boost::aligned_storage<sizeof(size_type), boost::alignment_of::value>::type;

In an example where impl_type has, say, 16 byte alignment and 1600 byte size.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/yet-another-user/pimpl/issues/1#issuecomment-323589157, or mute the thread https://github.com/notifications/unsubscribe-auth/ABswIPErX23ABiv583ifrvaurqrCkQVcks5saEUqgaJpZM4LhWw1 .

[glenfe]

Ah, right. :) In the same way that you allow the user of the type to supply size_type, is it then sensible to allow them to supply an align_type, to control alignment independently of size?

[yet-another-user]

Yes, it is indeed possible... and after reading your comment for a second I thought "why did not I think of it myself?". But then it seemed more and more as an overkill. I just wonder where/when I might need that flexibility? For ex., your example where impl_type has, say, 16-byte alignment and 1600 byte size. It'll all be aligned on the machine native "int" boundaries anyway. That is, 16 bytes alignment and 1600 byte alignments will result in the same initial address, right? Am I missing something?

[glenfe]

Well, std::aligned_storage_t<1600, 1600> (since you're supplying the 1600 for both size and alignment right now) is a little worrying in general, since 1600 is not a valid alignment. For example:

#include <type_traits>
int main()
{
    std::aligned_storage_t<1600, 1600> x;
}

With gcc/libstdc++ I (reasonably) see error: requested alignment is not a positive power of 2

But aside from this (even if the implementation of aligned_storage rounds 1600 up to the next valid alignment value), the concern would not be correctness but potential wasted space.

[yet-another-user]

Hmm, I tried myself (gcc-5.4.0-6ubuntu1~16.04.4):

boost::aligned_storage<1600, 1600> a;
boost::aligned_storage<1600, 16> b;
std::aligned_storage<1600, 1600> x;

All compiled and ran fine. A concern.

As for potentially wasted space, then how much space di you expect wasted? I suspect in any case it'll be aligned to the nearest machine "int", isn't?

Now that I looked into boost::aligned_storage, I am inclined to take the alignment out altogether and rely on the default alignment. I am just picturing having to ask people around here to supply an intelligent alignment value... can't see them happy. :-)

[glenfe]

For std::aligned_storage you need to use the type member (or the _t alias):

1. std::aligned_storage<1600, 1600>::type x;
2. std::aligned_storage_t<1600, 1600> x;

[glenfe]

I suspect in any case it'll be aligned to the nearest machine "int", isn't?

No. i.e. int is not required to be aligned at, say 256, on most C++ implementations that I know of (it is typically 4, 8, 16 on most common implementations). But if you specify alignas(256), or aligned_storage_t<N, 256> then it must give you storage that is at least 256 byte aligned.

[yet-another-user]

then it must give you storage that is at least 256 byte aligned.

If so, then it is wasteful. Agree.

[yet-another-user]

Should not it be: using storage_type = boost::aligned_storage<sizeof(size_type), alignof(size_type)>;

[glenfe]

Yes, that works too (i.e. allowing the one size_type to be a kind of size_and_alignment_type) - as it would also allow the user to specify both size and alignment.

[glenfe]

A more generic approach could just be changing the purpose of the template parameter from size_type to storage_type itself.

i.e. Let the user provide aligned_storage_t<X, Y> if they want for the storage_type template parameter.

[muggenhor]

Note that there are cases where alignment larger than the size of the type itself are sensible. Notably SSE can load (32-bit=4 byte/octet) floats significantly faster when they're aligned on 16-byte boundaries.

Additionally for some kinds of data (e.g. key material) locking a page in memory is required, as a result you generally want that data to be page-aligned. That's usually something like 4 or 8 KiB.

To make all of this possible I do think it's desirable to give the user the ability to control the alignment, while falling back to the default.