Subject: Re: [boost] [histogram] review part 3 (tests + summary)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-09-27 01:49:01
AMDG
I vote to ACCEPT histogram into Boost.
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.
In Christ,
Steven Watanabe