Re: [boost] [histogram] review part 2 (implementation)

On 26. Sep 2018, at 00:42, Steven Watanabe via Boost wrote: Destructors (along with constructors) are special.

"The lifetime of an object of type T ends when ... the destructor call starts" "The properties ascribed to objects throughout this International Standard apply for a given object only during its lifetime" [basic.life] (C++11)

And it gets worse. You're not just deallocating the buffer. You're also calling the destructors of all subobjects, including the label. The only thing that saves you from a double free is that you're using small labels that fit in the internal buffer in your tests.

Sorry, you are right. Additional proof: https://wandbox.org/permlink/CtfJSrGZi3jCAk58 Ok, no calling of destructors.

...

It is not overkill to use detail::buffer here instead of std::vector, since detail::buffer is anyway used by adaptive_storage. Replacing it here with std::vector does not make detail::buffer obsolete.

My real complaint is not so much overkill as the difficulty of getting manual memory management right, which makes the code more error-prone and harder to understand. I believe that the problems I have pointed out are a sufficient demonstration of this.

Point taken and I learned my lesson.

...

static and dynamic histograms have the exact same behaviour in this regard.

That may be what you intended, but it isn't what the code actually does. I can pass a one-element tuple to a dynamic histogram and it will be unpacked. Similarly, I can pass a (one element) vector to a (one axis) dynamic histogram and it will work. The latter has to be supported as the size cannot be detected at compile time.

Ok, that's a bug. I thought I unit-tested this case, but I didn't. Will be fixed. https://github.com/HDembinski/histogram/issues/106