std::valarray::sum does not work correctly for udt that has nonzero default value

• Key: STDCXX-693
• Type: Bug
• Status: Open
• Priority: Minor
• Assignee: Martin Sebor
• Reporter: Travis Vitek
• Component/s: 26. Numerics
• Affects Version/s: 4.2.0, 4.2.1
• Fix Version/s: 4.3.0

Description

#include <cassert>
#include <valarray>

struct S
{
// this ctor should not be required
S ()
: value (21)
{
}

S (int v)
: value (v)
{
}

S (const S& rhs)
: value (rhs.value)
{
}

S& operator+= (const S& rhs)

{ value += rhs.value; return *this; }

int value;
};

int main ()
{
const std::valarray<S> b (S (10), 1); // 10 elements with value 1
assert (b.sum ().value == 10);

return 0;
}

The wording in the standard seems to imply that the returned value should be a copy of one of the elements, and that op+= should be called on all of the other elements. If this is the case, then this an additional issue that would be detectable in user code [the user can count how many times the op+= is invoked]. This issue may apply to the min() and max() methods as well.

Comments

[Martin Sebor]

I agree that the wording in the standard does require it, although I suspect this interpretation was not intended (I could be wrong). GNU libstdc++ behaves the same as stdcxx, but STLport does what you expect. The straightforward fix is to copy the first element to the accumulator:

Index: include/valarray
===================================================================
--- include/valarray    (revision 609201)
+++ include/valarray    (working copy)
@@ -174,7 +174,7 @@
 
     // 26.3.2.7, p2
     value_type sum () const {
-        return accumulate (_C_array.begin (), _C_array.end (), value_type ());
+        return accumulate (_C_array.begin () + 1, _C_array.end (), _C_array [0]);
     }
 
     // 26.3.2.7, p3

Rather than implementing it, I would like to replace the use of std::accumulate() with a simple loop like so:

Index: include/valarray
===================================================================
--- include/valarray    (revision 609201)
+++ include/valarray    (working copy)
@@ -173,9 +173,7 @@
     }
 
     // 26.3.2.7, p2
-    value_type sum () const {
-        return accumulate (_C_array.begin (), _C_array.end (), value_type ());
-    }
+    value_type sum () const;
 
     // 26.3.2.7, p3
     value_type (min)() const {

Index: include/valarray.cc
===================================================================
--- include/valarray.cc (revision 609201)
+++ include/valarray.cc (working copy)
@@ -31,6 +31,18 @@
 
 
 template <class _TypeT>
+_TypeT valarray<_TypeT>::sum () const
+{
+    _TypeT __sum (_C_array [0]);
+
+    for (_RWSTD_SIZE_T __i = 1; __i < _C_array.size (); ++__i)
+        __sum += _C_array [__i];
+
+    return __sum;
+}
+
+
+template <class _TypeT>

[Travis Vitek]

Yes, I like that solution also. I've already done this for most of <valarray> for STDCXX-622 [which, I just noticed, appears to be a duplicate of STDCXX-512].

[Martin Sebor]

FWIW, I also did most of this as part of the "big valarray rewrite" last year. None of it is checked in because it's binary incompatible. That said, I have no problem committing improvements to valarray piecemeal, just as long as they're compatible, and leaving the breaking changes for last (5.0).

Since it sounds like we've both made some of the same changes it might make sense to compare our approaches. If you agree, I suggest you post your patch to the list before committing it to give me a chance to review it.

Incidentally, since we're outlining a function in the patch, it wouldn't be a forward compatible change, would it?

[Martin Sebor]

Scheduled for 4.3.

[Martin Sebor]

Estimated remaining effort.