HDembinski
commented
6 years ago Additional code from Steven.
friend.cpp
enum python_access;
#include <boost/histogram.hpp>one-element.cpp
#include <boost/histogram.hpp>
int main() {
using namespace boost::histogram;
auto h = make_static_histogram(axis::regular<>(4, 0, 1));
h(std::make_tuple(0.5));
}temporary-lifetime.cpp
#include <boost/histogram.hpp>
#include <iostream>
int main() {
using namespace boost::histogram;
auto h = make_static_histogram(axis::regular<>(4, 0, 1));
h(0.5);
for(auto x : h + h + h) {
std::cout << x.value() << std::endl;
}
}
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));
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:
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 weight(T&& t) {
return {t};
No forwarding? The same goes for sample.
axis/any.hpp:
242: explicit operator const base&() const { The name
baseis potentially confusing as there is also a base_type. I suggest qualifying it asaxis::baseaxis/base.hpp:
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?
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:
236: static_cast(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?
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:
detail/axes.hpp:
41: equal &= (tp && *tp == std::get(t));
This doesn't short-circuit on equal.
63: std::get(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& 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:
... Skip a few files with no comments. ...
storage/adaptive_storage.hpp:
299: b.set(b.template create(optr));
This isn't exception-safe (increaser does it right).
storage/operators.hpp: