Boost logo

Boost :

Subject: Re: [boost] Formal Review of Proposed Boost.Histogram Library Starts TODAY
From: Hans Dembinski (hans.dembinski_at_[hidden])
Date: 2018-09-18 18:55:58


>
> 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.
>

True, there is an inherent assumption that the transform does not change
the value_type and only do a monotonic mapping, like log(x) or pow(x,
const). But like you point out, this restriction is not necessary and it is
easy to avoid. But just out of curiosity, can you name a use case where the
transformed type is different from the value type?

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.

AFAIK, the order in which these statements are executed is not guaranteed,
that's why the ctor uses trans instead of transform_type. I am happy to be
proven wrong by a language expert. If that's the case, I agree that your
suggestion makes more sense.

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.
>

True, transforms were added late, I simply forgot to document them
properly. I added an issue to that effect.

 Kind regards,
Hans

On Tue, Sep 18, 2018 at 10:04 AM Gavin Lambert via Boost <
boost_at_[hidden]> wrote:

> 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.
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk