c++ for_each() and object functions

Question

I have an assignment that is the following:

For a given integer array, find the sum of its elements and print out the final result, but to get the sum, you need to execute the function for_each() in STL only once (without a loop).

As of now this is my code:

void myFunction (int i) {
cout << " " << i << " " <<  endl;
} 


int main() {

int array[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

vector<int> v(array[0], array[10]);

for_each( v.begin(), v.end(), myFunction);

return 0;
}

But for some reason the output shows up as 4198853, at first I thought it was a memory address but I figured out that was wrong. Any idea's as to what I might be doing wrong?

Answer 1

The array has 10 elements so 10 is not a valid array index.

vector<int> v(array[0], array[10]);
                              ^^

What you want is:

vector<int> v(array, array + sizeof(array) / sizeof(int) );

Answer 2

array has indexes 0..9, so array[9] = 10

if array[10] doesnt throw an error it will contain erroneous data, causing this problem.

Answer 3

vector<int> v(array[0], array[10]);

This doesn't do what you want. array[0] is the first value (1). array[10] is in invalid access past the end of your array. To pass pointers to the vector constructor, you want:

vector<int> v(array, array+10);

Answer 4

You need to take the address of array[0] and array[sizeof(array) / sizeof(*array)]. Vector constructor takes iterator types (i.e. pointers in this context), it can't magically determine the value of array[1] from the value of array[0].

Answer 5

In this line:

vector<int> v(array[0], array[10]);

You've indexed out of bounds of your array. This causes undefined behavior.

Also, the constructor for vector you used doesn't do what you think. You've used:

vector(initial value, count);

Answer 6

why not just:

for_each( array, array+10, myFunction);

I'm quite sure that int* can be used as iterator

EDIT: just checked this, it can indeed

Answer 7

You're not constructing the vector right. The constructor that takes two integers is

vector(size_type _Count, const Type& _Val);

So _Count is 1 and _Value is an undefined value past the end of the array.

Answer 8

Well, there's a couple problems beyond what people have said so far. One is your fault and the other is, in my opinion, a problem with the assignment.

You're printing out the elements, not the sum. The assignment asks for the sum so...you're doing it wrong. You need some call X that sums up all the values and sticks that into a variable for later printing.

The other problem is that std::for_each is not the appropriate algorithm for this task. In fact, it's so much not the appropriate algorithm that it's not even guaranteed to work without a lot of funky hacks to make all copies of the functor you pass in to for_each share the same counter. Maybe this is what your teacher wants you to figure out how to do, but I have a feeling (having experienced the common ability of programming instructors) that he/she doesn't actually know that they're teaching you wrong. The main gist of the problem is that implementations of std::for_each are free to make any number of copies of the function object passed in to recursive or utility calls to produce the standard behavior of for_each.

The appropriate algorithm to use is std::accumulate. In any production code I'd refuse to write, or accept from another team member, use of std::for_each to produce sums. However, I'd probably respond to this situation with a fugly hack and comment mentioning that for_each is the wrong algorithm. Something like so:

struct fugly_functor
{
  int * summation_variable; // using a local copy will result in correct answer, or a completely wrong answer depending on implementation of for_each

  fugly_functor(int * c) : counter(c) {}
  void operator(int x) { *summation_variable += x; }
};
...
int my_sum;
std::for_each(array, array+ELEM_COUNT, fugly_functor(&my_sum));
std::cout << my_sum << std::endl;

Then I'd suggest my teacher familiarize himself with the complete set of standard C++ algorithms.

The correct way would look something like so:

int my_sum = std::accumulate(array, array+ELEM_COUNT, 0);

Answer 9

If you really want to compute the sum, std::accucumulate is what you want, not for_each

Answer 10

I know you were told to use for_each, but I would actually do this - perhaps an alternative for extra credit ;-)

#include <numeric>
using namespace std;

int array[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

int sum = accumulate(array, array + (sizeof(array) / sizeof(int)), 0);

accumulate is expressly designed for summing the elements of an iterable range.