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

Dear Steven,

On 27. Sep 2018, at 03:49, Steven Watanabe via Boost 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