Boost logo

Boost :

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


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