Move 'null-state' behaviour to 'optional' and reject null-state behaviour in 'onstack'

[muggenhor]

This PR depends on #7.

This renames the current onstack policy to optional because it's behaviour closely models that of std::optional: provides internal storage, and optionally contains a value (or not: the null state). Then on top of that it implements a new onstack policy which explicitly rejects the null state at compile-time. This makes the onstack policy match, very closely, the behavior exhibited when no pimpl had been used at all. The only difference is that the size of this policy is still one pointer larger than that and possibly (likely) has larger alignment than necessary.

[yet-another-user]

Hmm, currently looking at your changes... What I do not think is growing on me is the onstack->optional rename. Trouble... Big... Huge!.. I can already see Andrzej Krzemienski (the guy maintaining boost::optional) tearing me to shreds. He was also very vocal about null-state API taken out of impl_ptr and explicitly made orthogonal to impl_ptr. In fact, I can imagine a number of people up in arms if we proceed with onstack->optional rename. It is because such a rename would shift the emphasis of the impl_ptr class from the Pimpl idiom (which happen to provide the null-state functionality) to the Optional idiom ((which has the null-state functionality as its core property). Don't you agree?.. I am open to any renames... but not "optional". :-)

[yet-another-user]

Uh, OK. Now I see where you are going... So, you rename onstack so that you could have a new onstack with no default (uninitialized) constructor. I see... So far I am far from convinced that such complication is necessary. I do understand that you want a "policy which explicitly rejects the null state at compile-time". That is,

OnStack::OnStack() : impl_ptr_type(nullptr) {}

will fail to compile. To be honest it appears like an overkill. Above the developer is not trying to ACCIDENTALLY create a null-state OnStack. He EXPLICITLY states that he wants a null-state OnStack by saying impl_ptr_type(nullptr). If you do not think your OnStack should support null-state, you simply never initialize impl_ptr_type as above... That is, the null-state functionality is indeed there... however, you are unlikely to deploy it accidentally. In other words, that additional precaution seems superfluous, an overkill. Don't you agree?

[yet-another-user]

I've incorporated the majority of your changes. Much appreciated. What did not sit well with me is the rename. So, I left the onstack name as-is. Still, I recognize that you might consider your no-null version of onstack important. If you find a compromise acceptable, then I suggest we leave the original onstack as-is and rename your new no-null onstack to something that you find meaningful, then I am happy to incorporate it. I personally do not feel it is necessary as I described above but it's just my view. Others (like you) might find such policy beneficial... if anything I find it to be a good example of policy customization. What do you think?

[muggenhor]

You're right, just not calling the nullptr_t constructor of base is enough to avoid the null-state. The additional benefit of having a separate type for that is that it communicates at the interface level that no null-state is provided. I.e. just not calling the nullptr_t constructor is something that's an implementation detail that's hidden from the user of a class, in this case very much by design (it's called "private implementation" for a reason ;-) ).

As for the optional name. I don't think it has to be a compilation naming conflict. There's no practical reason why the policies cannot have a namespace of their own. So the only potential problem I see here is that of user confusion. Given that it's always preceded by impl_ptr<A, AsizeT>:: I don't think that's very likely, because the context is always, explicitly, specified.

I had thought about always_onstack before as a name before, but after thinking how much like optional this is I thought that might be a better name precisely because of the similarity (except for using the traits_ pointer instead of a bool to indicate whether a value is stored it's the same storage wise).

Others (like you) might find such policy beneficial...

Yes, I find it useful because it has a name that's exposed at the interface level and as a result signals to the user that this type doesn't have an (implicit) null state to potentially worry about.

if anything I find it to be a good example of policy customization.

As an example for wanting to: yes. I did find one problem with adding policies though: currently they need to be added as member type aliases to boost_impl_ptr_detail. That practically means you need to edit the impl_ptr.hpp header to be able to add a policy. I think that's a rather serious deficiency, especially since in many places altering headers of third party libraries is an undesirable approach to extensibility.

[yet-another-user]

...I did find one problem with adding policies though: currently they need to be added as member type aliases to boost_impl_ptr_detail.

Good point. It's getting late down here in Australia. So, ​I just quickly addressed (or so I think) the above.​ User policies can now be specified/deployed as

​struct Book : boost::impl_ptr::policy ​ ​Here "detail::shared"​ is explicitly specified (say, user-provided custom) policy. Do you think it might be acceptable/sufficient?

[muggenhor]

This works, but with 8890bc71b487443b9ef5e64d7d6ce8b16d6fc838 you now specify the parameters differently for builtin policies and custom policies. That's a usage difference that would be nice to avoid (if possible).

[yet-another-user]

That's a fair point. So, I reverted it back to

struct OnStack : impl_ptr<OnStack, int[16]>::onstack struct OnStack : impl_ptr<OnStack, int[16]>::policy struct MyStack : impl_ptr<MyStack, int[16]>::policy