boostorg / histogram

Fast multi-dimensional generalized histogram with convenient interface for C++14
Boost Software License 1.0
315 stars 73 forks source link

Small question about sum string representation #266

Closed henryiii closed 4 years ago

henryiii commented 4 years ago

For the following example program:

#include <iostream>
#include <boost/histogram.hpp>

int main() {
    namespace bh = boost::histogram;
    bh::accumulators::sum sum(1);
    std::cout << sum << std::endl;
}

The printout here looks like it should be "sum(1 + 0)"

https://github.com/boostorg/histogram/blob/9e5a4c4f2406d84e068dbed7b070baa2c1a38930/include/boost/histogram/accumulators/ostream.hpp#L54-L60

However, it is not, but is just "1" (tested in wandbox with Boost 1.72). The other accumulators do print as expected, but sum does not, and I don't know why.

If this is the expected behavior, where is it coming from?

HDembinski commented 4 years ago

This is fixed in the develop branch now as part of some other changes. accumulators::sum used to be implicitly convertible to its value. You probably forgot to include the ostream.hpp header which has the operator<< defined for sum<T>. If it is not visible to the compiler, it tries to do the implicit conversion to get a match against operator<<(std::ostream&, double), which is defined in the iostream headers.

HDembinski commented 4 years ago

Now sum<T> is not implicitly convertible anymore and you will get a compile-time error instead.

HDembinski commented 4 years ago

If the header is visible, the ostreamer works also in 1.72, see the test here: https://github.com/boostorg/histogram/blob/master/test/accumulators_test.cpp#L174

HDembinski commented 4 years ago

Regarding your wandbox example, #include <boost/histogram.hpp> does not include the ostream.hpp's, as explained in the documentation. If we would include the ostream.hpp's, the user could not use the master include and write their own ostreamers.

henryiii commented 4 years ago

I'm not sure why boost-histogram was using the implicit conversion instead of the header, which was available, but this should avoid ambiguity. Thanks!