onstack policy: make alignment configurable

[muggenhor]

This makes alignment configurable, to give the user the option to choose something other than the default alignment, while simultaneously still permitting the user to rely on the (sane) default.

This fixes #1.

[yet-another-user]

You are one mean coding machine. :-) I can't keep up with you. :-) Much appreciated. You clearly know way more about stack-based implementation... How about you implement onstack as you see fit (alignment and all), put your name in the copyright (so that you get all the blame ;-)) and I'll include it as a whole. For that I'd only like to ask you to re-do this merge request to localize your onstack-related changes inside that onstack.hpp, i.e. move alignment-related stuff from detail.hpp to onstack.hpp... as it the only place where it is used, right? I see other policy classes changed slightly. I do not mind... although I do not see the immediate need for those changes. Could we address those separately please? Pretty please? :-)

If you dod decide to take over onstack, then it'll include the relevant test/impl_onstack.cpp, test/test.hpp and test/main.cpp.

What do you think?

[muggenhor]

You are one mean coding machine. :-) I can't keep up with you. :-) Much appreciated.

:-)

You clearly know way more about stack-based implementation...

I'm actually trying to use this as the base for writing an always_stack implementation that allows for the conditions sizeof(A) == sizeof(impl_ptr<A>::implementation) and alignof(A) == alignof(impl_ptr<A>::implementation): i.e. about as close to "zero overhead abstraction" as I can get. (See https://github.com/muggenhor/pimpl/blob/always_onstack/include/detail/always_onstack.hpp for my current attempt). That depends on not having the possibility for a null-state so must be a separate type.

How about you implement onstack as you see fit (alignment and all), put your name in the copyright (so that you get all the blame ;-)) and I'll include it as a whole.

Sure, sounds good.

For that I'd only like to ask you to re-do this merge request to localize your onstack-related changes inside that onstack.hpp, i.e. move alignment-related stuff from detail.hpp to onstack.hpp... as it the only place where it is used, right?

I see other policy classes changed slightly. I do not mind... although I do not see the immediate need for those changes. Could we address those separately please? Pretty please? :-)

I'm assuming you're referring to the addition of the typename... template parameters to the other policies? That was necessary because the number of template params caused problems for the policy type aliases. But that problem shouldn't exist anymore with your last change where only onstack gets its template params forwarded.

If you dod decide to take over onstack, then it'll include the relevant test/impl_onstack.cpp, test/test.hpp and test/main.cpp.

What do you think?

Sounds good.

[yet-another-user]

Excellent. I am sure it not needed but I'll mention anyway. :-) Please do not remove the null state from onstack. That way onstack can be a drop-in replacement for other policies having the null state. I understand you are building no-null always_onstack, so we'll include it as a separate policy. OK?

On Thu, Aug 31, 2017 at 9:25 PM, Giel van Schijndel < notifications@github.com> wrote:

You are one mean coding machine. :-) I can't keep up with you. :-) Much appreciated.

:-)

You clearly know way more about stack-based implementation...

I'm actually trying to use this as the base for writing an always_stack implementation that allows for the conditions sizeof(A) == sizeof(impl_ptr::implementation) and alignof(A) == alignof(impl_ptr::implementation): i.e. about as close to "zero overhead abstraction" as I can get. (See https://github.com/muggenhor/ pimpl/blob/always_onstack/include/detail/always_onstack.hpp for my current attempt). That depends on not having the possibility for a null-state so must be a separate type.

How about you implement onstack as you see fit (alignment and all), put your name in the copyright (so that you get all the blame ;-)) and I'll include it as a whole.

Sure, sounds good.

For that I'd only like to ask you to re-do this merge request to localize your onstack-related changes inside that onstack.hpp, i.e. move alignment-related stuff from detail.hpp to onstack.hpp... as it the only place where it is used, right?

I see other policy classes changed slightly. I do not mind... although I do not see the immediate need for those changes. Could we address those separately please? Pretty please? :-)

I'm assuming you're referring to the addition of the typename... template parameters to the other policies? That was necessary because the number of template params caused problems for the policy type aliases. But that problem shouldn't exist anymore with your last change where only onstack gets its template params forwarded.

If you dod decide to take over onstack, then it'll include the relevant test/impl_onstack.cpp, test/test.hpp and test/main.cpp.

What do you think?

Sounds good.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/yet-another-user/pimpl/pull/10#issuecomment-326267213, or mute the thread https://github.com/notifications/unsubscribe-auth/ABswIDpAkBZn2vB3srrQpuVc8s36Ymreks5sdpg7gaJpZM4PIm5y .

[muggenhor]

Excellent. I am sure it not needed but I'll mention anyway. :-) Please do not remove the null state from onstack. That way onstack can be a drop-in replacement for other policies having the null state.I understand you are building no-null always_onstack, so we'll include it as a separate policy. OK?

Exactly, my idea was to do it as a separate implementation so the user can choose what fits best (I'm not sure about the name always_onstack though, but renaming later on should be easy enough). I don't see an easy way to implement one on top of the other unfortunately. That does mean I need the alignment-related stuff twice. That's also why I placed it in detail.hpp.

[muggenhor]

FYI: Please don't merge this PR as right now I'm refactoring and adding some documentation and tests.

[muggenhor]

This PR is now ready to be merged (if you agree with its state of course).

For the always_onstack implementation I've currently got this, please tell me what you think: muggenhor/pimpl@3a23a1e87aa36bfa2ba86f100215e7e7336c9ff8 I'll probably make a PR out of that one tomorrow.

[yet-another-user]

The obvious just dawned on me -- the 'onstack' name is so-o-o embarrassingly wrong. It has nothing to do with the stack... Stack is purely incidental... only if the pimpl/proxy object happens to be created on the stack. If that pimpl/proxy object is created inside std::vector, then it's not on the stack. I am thinking, maybe, policy::noalloc, policy::inplace?........

[glenfe]

How is it in the crazy land of free?

It alternates between "can't complain", and "must try to not complain". :)

In general, I think users would most understand:

template<typename T, std::size_t Size, std::size_t Alignment = alignof(std::max_align_t)>
class Class;

i.e. They see they can specify size, and optionally specify alignment, otherwise it defaults to the maximum fundamental alignment. Anyone who needs extended alignment, would supply it explicitly. Note that the size and alignment values are specified as integers, similar to the template parameters of aligned_storage.

But, this simple interface could just be an alias template for the more generic one suggested above:

template<typename T, typename Storage>
class Class;

template<typename T, std::size_t Size, std::size_t Alignment = alignof(std::max_align_t)>
using Alias = Class<T, std::aligned_storage_t<Size, Alignment> >;

Just food for thought.

[yet-another-user]

It alternates between "can't complain", and "must try to not complain". :)

LOL. Do you get any time to code?.. From outside it appears like the whole country is busy toppling confederate statues... It percolated down here also... "revisiting" plaques to Cook and all. :-(

In general, I think users would most understand: template<typename T, std::size_t Size, std::size_t Alignment = alignof(std::max_align_t)> class Class;

Indeed. That seems the most intuitive, straightforward and non-controversial. There probably (hope, don't remember) was a reason I went with 'size_type'. Giel, do you think we could get back to the "roots" as Glen suggested? Don't you agree?

[yet-another-user]

...defaults to the maximum fundamental alignment. Anyone who needs extended alignment, would supply it explicitly.

Indeed. That resonates with what I ultimately ended up having for other policies. impl_ptr::shared, etc. are aliases for default deployments. When one needs to configure policies(say, custom allocator), it is done explicitly. Like

impl_ptr<Book, policy::shared, my_allocator>

[muggenhor]

Volodya, I was suggesting instead:

template<typename impl_type, typename storage_type>

i.e. Instead of independent size_type and align_type parameters that you are just going to use with aligned_storage, why not simply give the user the option to specify the storage type? They could supply aligned_storage_t<X, Y> as that type or something else.

I think it's better to instead pass on X and Y ourselves instead of depending on the user to get the complete storage type right. I'm using this in a situation where I am sure that not every colleague touching this code will understand alignment. And educating all of those isn't a realistic possibility as I cannot even know everyone involved. So two simple numbers (size and alignment, with a sensible default for the latter) are less error prone.

it fails to compile on my 64-bit -- static_assert in impl_onstack.cpp fails. Sizes reported are sizeof(OnStack)=48 and the other is 24. So, I erred on the safe side before merging.

Probably that's the result of std::string's exact size/alignment depending on whatever standard library we use. So we probably shouldn't have assertions on size/alignment of structs with those member types. Just changing the tested example (not to use types out of our control) should address that though.

I suggest in the docs we take it slowly.

Sure, I may have to mostly rewrite the "on-stack" section then, because I don't know how to do a gradual exhibition in the current structure. Writing documentation isn't as much my strong point as code though, so that'll take more time ;-)

In general, I think users would most understand:

template<typename T, std::size_t Size,
std::size_t Alignment = alignof(std::max_align_t)>
class Class;

Indeed. That seems the most intuitive, straightforward and non-controversial. There probably (hope, don't remember) was a reason I went with 'size_type'. Giel, do you think we could get back to the "roots" as Glen suggested? Don't you agree?

I do agree that passing the sizes/alignments directly, instead of passing types with those as properties, is preferable. The reason I kept it the same (with types instead of integers) is because the typename... more_types type pack cannot contain integers, only types. And as such forwarding of integer parameters isn't possible in that form. Although we could use std::integral_constant<IntegerType, Size/Alignment> instead. It's a bit more verbose, but other than that should provide the greater ease of use. We could even make that a template specialization, such that the current usage still works.

...defaults to the maximum fundamental alignment. Anyone who needs extended alignment, would supply it explicitly. Indeed. That resonates with what I ultimately ended up having for other policies. impl_ptr::shared, etc. are aliases for default deployments. When one needs to configure policies(say, custom allocator), it is done explicitly. Like

impl_ptr<Book, policy::shared, my_allocator>

I was actually thinking of something subtly different, specifically to do the same as the standard specifies for std::aligned_storage (meta.trans.other, 20.9.7.6 in C++11):

The value of default-alignment shall be the most stringent alignment requirement for any C ++ object type whose size is no greater than Len (3.9). The member typedef type shall be a POD type suitable for use as uninitialized storage for any object whose size is at most Len and whose alignment is a divisor of Align.

The requirements from the second sentence are already covered by the two static_asserts in the emplace function. The requirement for the default alignment is addressed by this PR by passing the buck to the underlying aligned_storage implementation. I think using std::aligned_storage might be a better fit than boost::aligned_storage for this, because the former implements a specific set of requirements, whereas I cannot find a description of the intent of the default alignment for the latter. It does however seem to equal alignof(std::max_align_t) which may be a wasteful default if you're storing a type smaller than that.

[muggenhor]

The obvious just dawned on me -- the 'onstack' name is so-o-o embarrassingly wrong. ... (snip) ... I am thinking, maybe, policy::noalloc, policy::inplace?........

Of those inplace seems the best to me. That may just be because it's the most similar sounding to 'placement new' which is effectively what we're using.

[muggenhor]

For example, adding this (as specializations inside the onstack struct of this PR):

    template <typename SizeInteger, SizeInteger size, typename AlignInteger, AlignInteger align>
    struct aligned_storage<std::integral_constant<SizeInteger, size>, std::integral_constant<AlignInteger, align>>
    {
        using type = boost::aligned_storage<size, align>;
    };

    template <typename SizeInteger, SizeInteger size>
    struct aligned_storage<std::integral_constant<SizeInteger, size>, void>
    {
        // void: explicitly ask for default alignment for this size
        using type = boost::aligned_storage<size>;
    };

Would enable this (the last integral constant can be left out and it'll pick a sensible default, the void case mentioned above):

struct A
    : boost::impl_ptr<A, impl_ptr_policy::onstack
      , std::integral_constant<std::size_t, sizeof(int) * 2>
      , std::integral_constant<int, alignof(int)>
      >
{
  A();
}

[yet-another-user]

struct A : boost::impl_ptr<A, policy::onstack , std::integral_constant<std::size_t, sizeof(int) * 2> , std::integral_constant<int, alignof(int)>

Ughm, I think I feel easier with

struct A : boost::impl_ptr<A, policy::onstack, int[2]>

which does the same as the scary version above. :-) ... That said, if you, guys, feel that something like "int[2]" does not cut it and it must be std::integral_constant<std::size_t, sizeof(int) * 2>, I'll trust your judgment.

The reason I kept it the same (with types instead of integers) is because the typename... more_types type pack cannot contain integers, only types.

Indeed, I remember now why I replaced size_t with size_type. So, it seems, keeping the size_type and specifying it like "int[64]" is the least evil of all choices... or so it seems to me, a non-alignment-and-all expert... But again, I'll trust your judgment.

... I may have to mostly rewrite the "on-stack" section then...

Sure thing. The section is yours. :-) Just think of your "colleagues who do not understand alignment". And no rush... as long as the exiting example in the docs is legit.

... it fails to compile on my 64-bit -- static_assert ...Probably that's the result of std::string's... ... So we probably shouldn't

Could you please update your branch/submission to reflect that so that I could merge it and get us in sync.

Of those inplace seems the best to me...

OK then. I guess, s/onstack/inplace/g will be in that update?.. Or I can rename later. Separately. No rush.

[yet-another-user]

Probably that's the result of std::string's exact size/alignment depending on whatever standard library we use. So we probably shouldn't have assertions on size/alignment of structs with those member types.

Isn't it a concern? All that new alignment-related complexity was added to make sure we create as frugal objects as possible. Then you added static_asserts to make sure that new alignment-related complexity works as expected. If seems to fail on my machine. So, now you seem to suggest to remove those static_asserts as the "frugality" cannot be ensured. If so, do we still need that new alignment-related complexity if we are not guaranteed to have what we expect/request/specify?

[muggenhor]

That said, if you, guys, feel that something like "int[2]" does not cut it and it must be std::integral_constant<std::size_t, sizeof(int) * 2>, I'll trust your judgment.

I can support both easily enough. I.e. most of the time for sizes you probably want to use types (because they're less verbose than integral_constant). Some times it may be necessary to have an easy way of specifying exactly what is necessary numerically, just having a specialization in the storage-selector template for integral_constant will permit that use-case too. So I'll just support both.

... it fails to compile on my 64-bit -- static_assert ...Probably that's the result of std::string's... ... So we probably shouldn't

Could you please update your branch/submission to reflect that so that I could merge it and get us in sync.

Yes, will do.

[muggenhor]

So, now you seem to suggest to remove those static_asserts as the "frugality" cannot be ensured. If so, do we still need that new alignment-related complexity if we are not guaranteed to have what we expect/request/specify?

Ah no, I was referring to this https://github.com/muggenhor/pimpl/blob/e384bc36a12d4341b13361647e2b77db343bede9/test/impl_onstack.cpp#L28 where we have an assertion checking the exact size instead of just the minimum size and alignment:

static_assert(sizeof(OnStack) == sizeof(boost::impl_ptr<OnStack>::implementation) + sizeof(void*),
"onstack, with a properly chosen size and alignment, should add only the size of a pointer to the implementation");

The static_asserts inside emplace ensuring the minima are met should definitely stay.

[glenfe]

If it needs to be types, and not sizes, then the following might resonate with users better:

struct A
    : impl_ptr<A, policy::in_place, size_traits<N, M> >

Instead of something like impl_ptr<A, policy::in_place, integral_constant<size_t, N>, integral_constant<size_t, M> >. You can invent a better name for that helper type.

template<size_t Size, size_t Alignment = default_alignment_v<Size> >
struct  size_traits{
    static constexpr size_t size = Size;
    static constexpr size_t alignment = Alignment;
};

But users specifying a type for which you would obtain sizeof and alignof from is much less idiomatic.

[yet-another-user]

size_traits

Seems very sensible to me... Now that you mention that, the int[64] I was advocating seems cryptic and hacky. :-)

[yet-another-user]

size_traits

... Although I'd think it'd be size_type and not size_traits. If I am not mistaken the "traits" meaning in English and as used in C++ relates to behavior... plural ("traits") especially. Here we seem to introduce a single type describing non-behavioral qualities.

[yet-another-user]

OK. 1) I renamed 'onstack' to 'inplace'... 'onstack' was wrong and I did not hear any other suggestions. Tried to rename to 'in_place' but it is already taken to provide the functionality of std::in_place for pre-C++17.

2) Per Glen's 'size_traits' suggestion I went ahead and implemented it (called policy::storage). It seems to take care of everything we discussed with ease. Funny now I can pass boost::aligned_storage as well... just as Glen was suggesting all along. :-) ... Maybe Glen was right. Giel, I'll leave it to you to decide.

3) Adjusted tests and documentation to match.

4) Giel, apologies. You'll probably have to revisit your pull request due to latest changes. I won't be surprised if I missed something. Alignment and all is not my forte.

[muggenhor]

Giel, apologies. You'll probably have to revisit your pull request due to latest changes. I won't be surprised if I missed something. Alignment and all is not my forte.

You completely solved the same problem I was trying to address, so this can be closed.