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&& operator*(histogram&& 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& 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::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(t));
This doesn't short-circuit on equal.
63: std::get(t) = static_cast(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()
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
