Refactor inplace policy

[muggenhor]

This refactors the inplace policy (and renames it to basic_inplace) such that the possibility of a null-state is no longer part of the size_type template type parameter. Instead this can now be controlled by an extra boolean has_null_state template parameter. We then construct two template aliases inplace and always_inplace which differ in the value they pass on for has_null_state to basic_inplace.

The storage difference is now handled by a combined 'storage and traits' class. basic_inplace selects the correct version of that class depending on the value of its has_null_state template parameter.

Additionally this splits allocation and construction in the traits classes. This allows the compiler (when it can devirtualize calls) to completely eliminate memory allocation code for basic_inplace based pimpls.

In addition to that I've also added a bugfix: we would leak memory if construction failed, because we never called deallocate in that case. That's handled now, albeit in a non-RTTI fashion.

[yet-another-user]

Nice catch ;-) of the construction-failure-related oversight. Merged with a few adjustments. Much appreciated. I've noticed you have two -- static_traits, local_traits -- classes. For the static_traits class traits_ptr is stored separately (in static) to achieve no-mem overhead. Interesting. I never thought of that. Any reason you still have local_traits instead of using static_traits for all? Do you think I should consider extending that technique onto 'copied' and 'unique'?

[muggenhor]

Nice catch ;-) of the construction-failure-related oversight.

I just realized we may have the same problem during destruction. I suspect memory should be deallocated even when throwing from the destructor of the implementation.

Any reason you still have local_traits instead of using static_traits for all?

Only because it currently does double duty as a flag to indicate that we're not in the null state (traits_ != nullptr). That was indeed my next idea for optimizing this.

Do you think I should consider extending that technique onto 'copied' and 'unique'?

I think the static pointer can probably be moved to be a member of the traits base class and give that class two functions. One to construct (only called in locations where impl_type is complete) and assign the pointer, another to retrieve the pointer value. If we ensure that we never call the retrieval function from within policies that don't have a constructed implementation yet then we should never get a nullptr back and be safe.

[yet-another-user]

... because it currently does double duty as a flag to indicate that we're not in the null state That was indeed my next idea for optimizing this.

Ah, indeed. I remember now. Then, I cannot immediately see how it can be optimized out. Something will have to replace it to fulfill the functionality.

... the static pointer can probably be moved to be a member of the traits base class and give that class two functions. One to construct (only called in locations where impl_type is complete) and assign the pointer, another to retrieve the pointer value.

Yes, I saw these get_traits()+set_traits() in you implementation... I have to say I usually try avoiding getters/setters... They blatantly violate encapsulation IMO. Let me think about it... Feel free to implement inplace as you see fit... just please don't rush changing other code to avoid disappointment. :-)

[yet-another-user]

I just realized we may have the same problem during destruction.

No, I do not think so. "destructors do not throw exceptions" is a standard lib. requirement. Violating it "is logically equivalent to violating a fundamental language rule"... Stroustrup pg. 354

[muggenhor]

I just realized we may have the same problem during destruction.

No, I do not think so. "destructors do not throw exceptions" is a standard lib. requirement. Violating it "is logically equivalent to violating a fundamental language rule"... Stroustrup pg. 354

For containers that makes sense, this however, is basically a smart pointer, and I think destruction should be closer to that of operator delete. From [expr.delete] 5.3.5, paragraphs 6 and 7:

6. If the value of the operand of the delete-expression is not a null pointer value, the delete-expression will invoke the destructor (if any) for the object ...
7. If the value of the operand of the delete-expression is not a null pointer value, the delete-expression will call a deallocation function (3.7.4.2). ... [Note: The deallocation function is called regardless of whether the destructor for the object ... throws an exception. --- end note]

So if we want to resemble that we do need to ensure that deallocation happens regardless of an exception being thrown. If not we may as well mark the method destroy of the traits as noexcept and turn undefined behaviour (possibly leaking memory) into defined behaviour (terminate). Personally I prefer matching delete's semantics.

[yet-another-user]

Hmm. what you are quoting makes sense... On the other hand my quote from Stroustrup was not about containers-only... but I do feel like agreeing with you... Let me double check it tomorrow.

[yet-another-user]

Giel, I see you fixed a bug that I introduced in unique and added a test for it. Much appreciated. It's disappointing that I missed that. It took me a while to realize that you fixed it so promptly right in the master branch -- I was looking at the code thinking "wait a minute, I remember changing this piece" :-). I've created two branches -- gs and vb -- for us so that "master" will be less susceptible to issues like I introduced (and you had to fix). What do you think?

[yet-another-user]

If not we may as well mark the method destroy of the traits as noexcept and turn undefined behaviour (possibly leaking memory) into defined behaviour (terminate). Personally I prefer matching delete's semantics.

Looked at boost/smart_ptr/make_shared_object.hpp. It implements the first (noexcept) version. Then, std::allocator does not do try/catch either... even though I'd expect its destroy() would be the perfect place to have try/catch. Then 15.2.3 indicates that "destructors should generally catch exceptions and not let them propagate out of the destructor" as otherwise hell breaks loose (during "stack unwinding"). So far, it appears try/catch around destruct() does not really provide solid guarantee. So, that's why (I think) shared_ptr's destroy() opted for noexcept. What do you think?

[muggenhor]

std::allocator cannot do anything sensible because neither it nor std::allocator_traits have a function that is supposed to handle both destruction and deallocation. I think allocate_shared may be a better example, because it has to handle almost the same situation. I did look at glibc++ for a bit but didn't have enough time to fully investigate, but it looked like it handled the memory through RAII, so that would probably still get deallocated, even when the destructor throws.

[muggenhor]

Looking at [util.smartptr.shared.const] paragraph 8 it seems to suggest that a shared pointer's deleter should not throw. So modelling it like that and marking as noexcept seems okay.

[muggenhor]

After 2ee3191c392a8846abeb5c6408a11ca5c237ec7d it's a moot issue because that uses RAII always to deallocate. Meaning it's safe for both throwing constructors and throwing destructors with no additional effort for either.