On 17/09/2018 21:06, Mateusz Loskot wrote:
The formal review of Hans Dembinski's Histogram library starts TODAY,
[...]
The author followed the "Don't pay for what you don't use" principle.
[...]
Documentation: http://hdembinski.github.io/histogram/doc/html/
Perhaps this idea falls into that principle, but there seems to be an
inherent assumption in the axis classes that a transform will always
return the same type as its input.
(For example, the min_ and delta_ fields in the regular axis are stored
as value_type, as are the input arguments lower and upper.)
It seems like it should be trivial to "fix" that, though, by storing
these already-transformed values as type transformed_type instead.
using transformed_type =
decltype(std::declval().forward(std::declval()));
Or similar. This should in theory allow an arbitrarily complex
value_type to be used, provided that a suitable transform can provide
bidirectional conversion to a simpler numeric type.
On a related note, the method used to store the transform as a base
class of the axis bothers me a bit, in particular that it calls the copy
constructor to init the base rather than the move constructor, and that
the init of min_ and delta_ (for axis::regular) are invoked on the
original copy instead of the internal copy which is subsequently used
for all other operations.
I'm not 100% clear on the standards guarantees for the constructor
initialiser list (in particular if a base initialiser is guaranteed to
be fully constructed before the arguments to other initialisers are
evaluated or not), so it's possible that this is UB if that guarantee is
not met, but I would have thought it'd be more sensible to init as:
regular(...)
: transform_type(std::move(trans)
, min_(transform_type::forward(lower))
, ...
Such that the same instance is used in all cases.
Also: transforms don't seem to be mentioned much in the documentation.
I couldn't find any documented requirements to build your own, for
example, though I worked it out from looking at the source.
