Boost logo

Boost :

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


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk