Re: [boost] [histogram] review part 2 (implementation)

Subject: Re: [boost] [histogram] review part 2 (implementation)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-09-25 00:22:49

AMDG

arithmetic_operators.hpp:

16: Returning an rvalue reference is tempting, but it
  can cause issues in some cases because it prevents
  the lifetime of the temporary from being extended in
  certain cases.  See temporary-lifetime.cpp.

42: histogram<A, S>&& operator*(histogram<A, S>&& a, const double x)
  histogram::operator*= uses scale_type.  I don't really
  care whether you use scale_type or double, but please
  be consistent.  If you're hard-coding double here, then
  there's no point to making the internals more flexible.

histogram.hpp:

36: What are the requirements on the Axes parameter?
    I'm deducing that it must be either a std::tuple
    or a std::vector containing Axes, but I didn't
    see this explicitly stated anywhere.

60: histogram& operator=(const histogram<A, S>& rhs)
  This isn't exception safe unless axes_assign is
  nothrow--which it isn't.

107: std::size_t dim() const noexcept
  This can probably be constexpr as well.

207: // precondition: argument sequence must be strictly ascending axis
indices
  This is exactly the sort of information that needs to
  appear in the documentation.  Use \pre.
  Also it would be nice if this weren't a requirement,
  and reduce_to could shuffle the axes.
  Does/should it work in the degenerate case of
  reducing a histogram to itself?

234:  BOOST_ASSERT_MSG(begin == end ||
  Will reduce_to even work if begin == end?
  If nothing else, I think it will fail in the do/while on L247
  Also, you're only checking the end, but the code seems
  to permit signed integers.  (Please specify the restrictions
  on the iterator's value_type clearly).

261:  friend class python_access;
  I don't see a forward declaration of this.  Please note
  that the semantics of this can be exceedingly strange
  when there is no prior declaration. See friend.cpp

321: auto axes = typename H::axes_type(s.get_allocator());
     axes.reserve(std::distance(begin, end));

     while (begin != end) axes.emplace_back(*begin++);
  This can all be reduced to one line:
     auto axes = typename H::axes_type(begin, end, s.get_allocator())

histogram_fwd.hpp:

N/A

iterator.hpp:

27: iterator_over(const iterator_over& o)
  It's probably worth a comment that the cache is not copied,
  as I almost suggested defaulting this.

66: void advance(int n)
  The difference_type is ptrdiff_t.

- ForwardIterators and higher must be default constructable.

- distance_to is not implemented.

literals.hpp:

- The standard guarantees that the digits are consecutive,
  so char2int can be just (C - '0').

ostream_operators.hpp:

- You don't actually need ostream.  iosfwd is enough,
  especially since you're only #including histogram_fwd.hpp,
  and not histogram.hpp.

- It looks like the format includes a trailing comma, which
  seems a little odd when you go to the trouble of not adding
  a new line for an empty histogram.

serialization.hpp:

77: if (Archive::is_loading::value) { S::apply(typename S::destroyer(),
s.buffer); }
  This isn't exception-safe, as creating a new buffer can
  throw leaving a dangling pointer behind.

128: if (Archive::is_loading::value) { this->~variable(); }
  Don't call the destructor like this.

- This does not support XML archives.

- serialize for user defined axis types seems
  error prone, because labelled_base::serialize
  is inherited.  If the user forgets to add serialize,
  the code will compile and silently slice the object.

weight.hpp:

12: namespace detail {
  Be careful about exposing types from namespace detail
  in any way.  It allows ADL in client code to look inside
  your detail namespace.

26: detail::weight_type<T> weight(T&& t) {
      return {t};
  No forwarding?  The same goes for sample.

axis/any.hpp:

242: explicit operator const base&() const {
  The name `base` is potentially confusing as there is also
  a base_type.  I suggest qualifying it as `axis::base`

axis/base.hpp:

- I only see operator==, but not operator!=.

axis/interval_view.hpp:

33: return axis_.lower(idx_ + 1);
  The requirement that an axis needs to provide lower to
  work with interval_view is not documented.  Neither
  is the exact meaning of lower.  In particular, it needs
  to work with a past-the-end index.  The actual implementations
  seem to work for any index, but I'm not sure that the
  result is actually meaningful if the index is out-of-bounds.

39: operator==
  Should two interval_views be considered to be the same or
  different if they refer to distinct, but equal axes?

- Actually, I find interval_view somewhat unconvincing.
  Why can't it just be a pure value that stores the upper
  and lower bounds?  The value_type is usually going to
  be a built-in type anyway, not to mention that you
  haven't documented interval_view sufficiently for
  it to be used by user-defined axes.

axis/iterator.hpp:

- You're using the default difference_type = ptrdiff_t,
  but advance and distance_to use int.

- Does a separate reverse_iterator_over have any benefit over
  std::reverse_iterator?

- It seems inconsistent for axes to have rbegin/rend, when
  histogram itself doesn't.

axis/ostream_operators.hpp:

22: Calling the function to_string is somewhat misleading,
  because what you're actually getting is a string formatted
  for appending to something else.

26: escape_string doesn't escape `\`.

138: Should this specialize for basic_string rather than
  just string?

axis/types.hpp:

- The interface for a Transform isn't documented.

236: static_cast<int>(std::floor(z * base_type::size()))
  The cast to int might overflow for large values of z.

332,350: this->~variable();
  Don't call the destructor in the assignment operator.

435: if (i < 0) { return -std::numeric_limits<value_type>::max(); }
  Did you mean min?

- Is there a reason that you're not just using std::vector
  for variable and category?

429: const int z = x - min_;
  Both x and min_ have type value_type, not int.

511+529: this->~category();
  Just no.

568: mutable int last_ = 0;
  mutable is dangerous as it means that non-mutating
  functions cannot safely be run concurrently without
  syncronization.  (The other place that uses mutable,
  iterator_over, is safer because iterators should
  usually not be shared in the first place.)

axis/value_view.hpp:

- As with interval_view, I don't see any need to keep
  a reference to the axis.

detail/axes.hpp:

- Don't use the unnamed namespace in headers.

41: equal &= (tp && *tp == std::get<N::value>(t));
  This doesn't short-circuit on equal.

63: std::get<N::value>(t) = static_cast<const T&>(v[N::value]);
  Using an explicit conversion operator may not be the best choice
  here.  My first reaction was to flag this as potential UB
  because of the unchecked cast.  I had to look through
  the definition of any to realize that it is in fact
  checked and can throw.

122: t.resize(sizeof...(Us));
  reserve+push_back is slightly better than resize+assign.

220: value *= t.shape();
  It might be wise to check for integer overflow here.

521: optional_index call_impl(Tag, const std::tuple<T>& axes,
  It seems that this specialization prevents using a single
  element container with a single element static histogram,
  which seems inconsistent with the documentation and the
  behavior for dynamic histograms.

detail/buffer.hpp:

42: You're assuming that the iterator operations themselves
  do not throw, which isn't guaranteed.

86: std::is_nothrow_constructible<T, U>()
  This doesn't match the constructor call.

detail/cat.hpp:

- unnamed namespace again.

... Skip a few files with no comments. ...

storage/adaptive_storage.hpp:

299: b.set(b.template create<T>(optr));
  This isn't exception-safe (increaser does it right).

storage/operators.hpp:

- These generic comparison operators seem really
  dangerous unless you make detail::requires_storage
  much more precise.

In Christ,
Steven Watanabe