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

Subject: Re: [boost] [histogram] review part 2 (implementation)
From: Hans Dembinski (hans.dembinski_at_[hidden])
Date: 2018-09-26 08:17:32

> On 26. Sep 2018, at 00:42, Steven Watanabe via Boost <boost_at_[hidden]> 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