Re: [boost] Formal Review of Proposed Boost.Histogram Library Starts TODAY

Subject: Re: [boost] Formal Review of Proposed Boost.Histogram Library Starts TODAY
From: Gavin Lambert (boost_at_[hidden])
Date: 2018-09-18 08:04:08

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<transform_type>().forward(std::declval<value_type>()));

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.