Re: [boost] [histogram] review part 3 (tests + summary)

Subject: Re: [boost] [histogram] review part 3 (tests + summary)
From: Hans Dembinski (hans.dembinski_at_[hidden])
Date: 2018-09-27 08:43:05

Dear Steven,

> On 27. Sep 2018, at 03:49, Steven Watanabe via Boost <boost_at_[hidden]> wrote:
> 
> I vote to ACCEPT histogram into Boost.

thank you for the exceptionally thorough reading of the code and the documentation. I am really impressed how much you found by just reading the code. I believe you invested a significant fraction of your personal time into this review. So I feel I owe you one.

> Critical issues:
> - Various exception safety and memory issues must
>  be fixed.
> - The reference documentation must be completed.
>  Specifically, various preconditions, requirements,
>  and assumptions must be made explicit.
> 
> Tests (I'm running out of time, so this is a bit incomplete):
> 
> - Some of run-fail tests would be better off as
>  just testing that the correct exception is thrown.
>  (i.e. BOOST_TEST_THROWS).  The ones that assert
>  must force debug builds (<variant>debug) as
>  assertions are a no-op in release builds.
> 
> - For serialization tests, I think that you should
>  save a text archive permanently.  This will allow
>  you to detect cases where the serialization format
>  changes.  (Pass the archive on the command line.
>  The `run` rule in b2 has an `args` parameter.)
> 
> utility.hpp:115: void deallocate(T*& p, std::size_t n) {
>  Don't take the argument by reference.  It's not guaranteed to work.
> 
> axis_test.cpp:489: BOOST_TEST_EQ(db.size(), 5); // axis_type, char,
> double, long + ???
>  The ??? is an internal structure created by vector
>  to support checked iterators (specifically it's
>  std::_Container_proxy).  This test will fail in
>  release builds.
> 
> index_mapper_test.cpp:
>  I feel that this test is a bit too specific.  The important
>  part is the relationship between linearize/indices_to_index
>  (detail/axes.hpp), index_cache, and index_mapper.
>  The exact mapping doesn't matter from the point of
>  view of program correctness, as long as they are all
>  consistent.

I will fix these issues and the suggestions all make sense.

Best regards,
Hans