yet-another-user / pimpl

The C++ Pimpl Idiom Again!
36 stars 7 forks source link

Always inplace #11

Closed muggenhor closed 7 years ago

muggenhor commented 7 years ago

This moves the decision on whether to have a null state to the size_type template parameter of inplace. Specifically it requires two extra members in that template parameter: has_null_state, which must be a constexpr bool, that is true if and only if a null-state is permitted. Secondly it contains a type member which becomes the new storage type. It contains both the storage and handles storage of the traits pointer. It handles the traits pointer in such a way that this pointer is eliminated from the object when no null state is permitted, thus allowing zero memory overhead.

This builds on #10.

yet-another-user commented 7 years ago

I go to bed. Come back. New code. I like it. :-) I corrected "derived_type" in a couple of places. Merged. Thank you. One thing. Not a criticism. Just asking. You chose to deploy always_inplace functionality via always_storage parameter. I thought you favored the explicitness and non-ambiguity of always_inplace... Something that also seemed straightforward and friendly to me. Now, it appears, instead of saying "always_inplace" we say "inplace+always_storage". Are you sure it is a better way to communicate the purpose to the user? Maybe "always_storage" name could be improved... or maybe the original separate "always_inplace" policy was easier to communicate?

Another nit is that I see you merged traits functionality into the storage functionality... There might be a practical reason for that... However, from the design point it appears like unnecessary coupling/complexity... storage and traits functionality seem orthogonal/unrelated and my inclination would be to see them separate. What do you think?

muggenhor commented 7 years ago

I corrected "derived_type" in a couple of places.

Oops, that was a copy-paste mistake. Thanks for finding it!

You chose to deploy always_inplace functionality via always_storage parameter. I thought you favored the explicitness and non-ambiguity of always_inplace... Something that also seemed straightforward and friendly to me.

Yes, naming is still an issue here. I had first tried another approach, with the specializations that handle the differing traits as two deriving classes. That got rather ugly rather fast however. On the outside it had the two nice names though.

Another nit is that I see you merged traits functionality into the storage functionality... There might be a practical reason for that... However, from the design point it appears like unnecessary coupling/complexity... storage and traits functionality seem orthogonal/unrelated and my inclination would be to see them separate. What do you think?

It's actually the other way around: I moved the storage functionality into the traits class. I.e. I started by moving out the storage of the traits class and using the empty-base optimization to ensure the 'always' policy didn't incur any extra bytes. That caused the storage to be moved after the traits pointer for the "optional" version though. This caused the "check that implementation is allocated on the stack" test to fail. And for storage types with an alignment requirement larger than the size of a pointer it would cause padding to be added in between.

I don't like the coupling either, I prefer to put the optional/always difference in the policy itself. It may be possible to accomplish this with an intermediate helper class that stores both the trait pointer and storage area. That needs some thought and experimentation though, after I get some sleep first.

In summary: I think both the coupling and naming are caused by the same problem. I think they're solvable, but that'll take some time and effort.

yet-another-user commented 7 years ago

Do you ever sleep? :-)

Yes, naming is still an issue here... I don't like the coupling either... I prefer to put the optional/always difference in the policy itself.

My personal preference would still be two separate policies (from the user angle)... seems like the cleanest... even though they might be specializations of the same underlying policy.

...but that'll take some time and effort.

Understood. Take all time and effort you need... Will it be ready by yesterday? ;-) Sorry, could not resist. :-) That's what some of my managers lo-o-o-ve saying... after a long discussion of a serious component they'd always say this stupid "joke"... And I think, yeah, that was mildly funny a year ago when I heard that for the first time. Sigh.