fixing bug in detection of weight support in user-defined accumulators (#314)

* Also fixed the accumulator concept spec, which was wrong

doc/concepts/Accumulator.qbk

@@ -21,7 +21,7 @@ An [*Accumulator] is a functor which consumes the argument to update some intern
   [`a(ts...)` or `++a`]
   []
   [
-    Either a call operator accepting a fixed number of arguments must be implemented, or the pre-increment operator. The call operator may not be templated and not overloaded, except to support weights as described under optional features.
+    Either a call operator accepting a fixed number of arguments must be implemented, or the pre-increment operator. The call operator form `a(ts...)` may not be templated and not overloaded, except to support weights as described under optional features.
   ]
 ]
 [
@@ -44,17 +44,16 @@ An [*Accumulator] is a functor which consumes the argument to update some intern
 
 * `A` is a type meeting the requirements of [*Accumulator]
 * `a` and `b` are values of type `A`
-* `w` is a value of type [classref boost::histogram::weight_type], where `T` is a number type
+* `w` is a value of type [classref boost::histogram::weight_type]
 * `ts...` is a pack of values of arbitrary types
-* `v` is a number value (integral or floating point)
 
 [table Valid expressions
 [[Expression] [Return] [Semantics, Pre/Post-conditions]]
 [
-  [`a += v` or `a(w, ts...)`]
+  [`a += w` or `a(w, ts...)`]
   []
   [
-    Does a weighted fill of the accumulator. Use this to implement weight support for an accumulator that is normally filled with `++a` or `a(ts...)`, respectively. Only the corresponding matching form may be implemented: `a += v` for `++a`, `a(w, ts...)` for `a(ts...)`. The implementations may not be templated and not overloaded.
+    Does a weighted fill of the accumulator. Use this to implement weight support for an accumulator that is normally filled with `++a` or `a(ts...)`, respectively. Only the corresponding matching form may be implemented: `a += w` for `++a`, `a(w, ts...)` for `a(ts...)`. The call operator form `a(w, ts...)` may not be templated and not overloaded.
   ]
 ]
 [

doc/history.qbk

@@ -12,6 +12,7 @@
 * Fixes
   * `algorithm::reduce` with `crop` now does not remove the counts in flow bins anymore if the selected range actually overlaps with the flow bins, making the treatment of flow bins consistent with inner bins
   * `accumulators::mean` and `accumulators::weighted_mean` now compute the correct variance when `operator+=` was called, for example, when histograms with accumulators are added; this was wrong before leading to too small variances
+  * detection of weight support in user-defined accumulators was broken at compile-time if accumulator used `operator+=` instead of `operator()`; tests only accidentally passed for builtin `weighted_sum`
 
 [heading Boost 1.75]
 

include/boost/histogram/accumulators/weighted_sum.hpp

@@ -44,8 +44,7 @@ public:
   }
 
   /// Increment by weight.
-  template <class T>
-  weighted_sum& operator+=(const weight_type<T>& w) {
+  weighted_sum& operator+=(const weight_type<value_type>& w) {
     sum_of_weights_ += w.value;
     sum_of_weights_squared_ += w.value * w.value;
     return *this;

include/boost/histogram/detail/accumulator_traits.hpp

@@ -51,9 +51,15 @@ template <class R, class T, class... Ts>
 accumulator_traits_holder<false, Ts...> accumulator_traits_impl_call_op(R (T::*)(Ts...));
 
 template <class T>
-auto accumulator_traits_impl(T&, priority<1>)
+auto accumulator_traits_impl(T&, priority<2>)
     -> decltype(accumulator_traits_impl_call_op(&T::operator()));
 
+template <class T>
+auto accumulator_traits_impl(T&, priority<2>)
+    -> decltype(std::declval<T&>() += std::declval<boost::histogram::weight_type<int>>(),
+                accumulator_traits_holder<true>{});
+
+// fallback for simple arithmetic types that do not implement adding a weight_type
 template <class T>
 auto accumulator_traits_impl(T&, priority<1>)
     -> decltype(std::declval<T&>() += 0, accumulator_traits_holder<true>{});
@@ -64,7 +70,7 @@ auto accumulator_traits_impl(T&, priority<0>) -> accumulator_traits_holder<false
 // for boost.accumulators compatibility
 template <class S, class F, class W>
 accumulator_traits_holder<false, S> accumulator_traits_impl(
-    boost::accumulators::accumulator_set<S, F, W>&, priority<1>) {
+    boost::accumulators::accumulator_set<S, F, W>&, priority<2>) {
   static_assert(std::is_same<W, void>::value,
                 "accumulator_set with weights is not directly supported, please use "
                 "a wrapper class that implements the Accumulator concept");
@@ -72,7 +78,7 @@ accumulator_traits_holder<false, S> accumulator_traits_impl(
 
 template <class T>
 using accumulator_traits =
-    decltype(accumulator_traits_impl(std::declval<T&>(), priority<1>{}));
+    decltype(accumulator_traits_impl(std::declval<T&>(), priority<2>{}));
 
 } // namespace detail
 } // namespace histogram

test/detail_accumulator_traits_test.cpp

@@ -15,6 +15,12 @@ namespace dtl = boost::histogram::detail;
 int main() {
   using boost::histogram::weight_type;
 
+  BOOST_TEST(dtl::accumulator_traits<int>::weight_support);
+  BOOST_TEST_TRAIT_SAME(dtl::accumulator_traits<int>::args, std::tuple<>);
+
+  BOOST_TEST(dtl::accumulator_traits<float>::weight_support);
+  BOOST_TEST_TRAIT_SAME(dtl::accumulator_traits<float>::args, std::tuple<>);
+
   struct A1 {
     void operator()();
   };
@@ -68,15 +74,36 @@ int main() {
   BOOST_TEST(dtl::accumulator_traits<A7>::weight_support);
   BOOST_TEST_TRAIT_SAME(typename dtl::accumulator_traits<A7>::args, std::tuple<int&&>);
 
-  struct B {
+  struct B1 {
     int operator+=(int);
   };
 
-  BOOST_TEST(dtl::accumulator_traits<B>::weight_support);
-  BOOST_TEST_TRAIT_SAME(typename dtl::accumulator_traits<B>::args, std::tuple<>);
+  BOOST_TEST(dtl::accumulator_traits<B1>::weight_support);
+  BOOST_TEST_TRAIT_SAME(typename dtl::accumulator_traits<B1>::args, std::tuple<>);
 
-  BOOST_TEST(dtl::accumulator_traits<int>::weight_support);
-  BOOST_TEST_TRAIT_SAME(dtl::accumulator_traits<int>::args, std::tuple<>);
+  struct B2 {
+    int operator+=(weight_type<int>);
+  };
+
+  BOOST_TEST(dtl::accumulator_traits<B2>::weight_support);
+  BOOST_TEST_TRAIT_SAME(typename dtl::accumulator_traits<B2>::args, std::tuple<>);
+
+  struct B3 {
+    int operator+=(const weight_type<double>&);
+  };
+
+  BOOST_TEST(dtl::accumulator_traits<B3>::weight_support);
+  BOOST_TEST_TRAIT_SAME(typename dtl::accumulator_traits<B3>::args, std::tuple<>);
+
+  // potentially ambiguous case that mimicks accumulators::weighted_sum
+  struct B4 {
+    B4(int) {}
+    int operator+=(const weight_type<double>&);
+    int operator+=(const B4&); // *this += 0 succeeds as *this += B4(0)
+  };
+
+  BOOST_TEST(dtl::accumulator_traits<B4>::weight_support);
+  BOOST_TEST_TRAIT_SAME(typename dtl::accumulator_traits<B4>::args, std::tuple<>);
 
   return boost::report_errors();
 }