C++ vector std::bad_alloc error in a program

Question

I was trying to sort a vector having user defined data type having two values, on the basis of one. But i get the error of bad_alloc. this is the code :

#include<iostream>
#include<vector>
#include<algorithm>

using namespace std;

struct s{
int value;
int weight;
};

bool comp(s a , s b){
return a.value>b.value;
}

int main(){
vector <s> p;
s temp;
int n;
cin>>n;
while(n>=1){
    cin>>temp.value;
    cin>>temp.weight;
    p.push_back(temp);
}
sort(p.begin(), p.end(), comp);
vector <s> :: iterator it;
for(it = p.begin(); it != p.end();it++){
    *it = temp;
    cout<<temp.value<<endl;
}

}

on running :

terminate called after throwing an instance of 'std::bad_alloc' what(): std::bad_alloc

can anyone help?

`std::vector::push_back()` reallocates memory whenever it's buffer is out of capacity. This and the infinite loop mentioned already by songyuanyao _must_ end with a `std::bad_alloc` after a sufficient number of iterations. What I do not understand: How do you provide so much input data that the "out of memory" (aka bad_alloc) can happen? — Scheff's Cat, Jun 08 '17 at 05:51

score 2 · Accepted Answer · answered Jun 08 '17 at 05:54

Problems that I see:

Infinite loop

In the loop

while ( n >= 1)
{
   ...
}

you are not changing the value of n. If n is greater 1 than at start of the loop, the loop will never end.

Not checking the status of input

You have

 cin >> temp.value;
 cin >> temp.weight;

You are not checking whether those calls succeeded. You are assuming that they did and proceeding to use temp.

Wrong way assignment

In the final loop, you are using

*it = temp;

That will change the vector, not extract the value from the vector.

Here's an updated version of main that should work.

int main()
{
   vector <s> p;
   s temp;
   int n;
   cin>>n;
   while(n>=1)
   {
      // If there is a problem reading, break.
      if ( !(cin>>temp.value) )
      {
         break;
      }

      // If there is a problem reading, break.
      if ( !(cin>>temp.weight) )
      {
         break;
      }

      p.push_back(temp);

      // Decrement n
      --n;
   }

   sort(p.begin(), p.end(), comp);
   vector <s> :: iterator it;
   for(it = p.begin(); it != p.end();it++)
   {
      // Extract the value from the vector and print it.
      temp = *it;
      cout<<temp.value<<endl;
   }
}

Remy Lebeau · Answer 2 · 2017-06-08T06:15:44.763

If the user enters a value of 1 or higher for n, the loop never ends, filling the vector until it uses up all available memory. You are not decrementing n on each loop iteration so the loop eventually breaks:

while (n >= 1) {
    cin >> temp.value;
    cin >> temp.weight;
    p.push_back(temp);
    --n; // <-- add this
}

In this situation, a for loop would be more appropriate than a while loop:

for (int i = 0; i < n; ++i) {
    cin >> temp.value;
    cin >> temp.weight;
    p.push_back(temp);
}

I would go as far as getting rid of the manual loop altogether by defining a custom operator>> for struct s and then use std::copy_n() with std::istream_iterator and std::back_inserter:

#include <iostream>
#include <algorithm>
#include <iterator>

istream& operator>>(istream &in, s &out) {
    in >> out.value;
    in >> out.weight;
    return in;
}    

int main() {
    ...
    int n;
    cin >> n;
    copy_n(istream_iterator<s>(cin), n, back_inserter(p));
    ...
}

Any way you fill the vector, your comp function should take its input parameters by reference:

bool comp(s &a, s &b) {
    return a.value > b.value;
}

Also, your output loop is not using the iterator correctly. You need to get rid of the *it = assignment and just output the referenced value as-is:

for(it = p.begin(); it != p.end(); ++it) {
    //*it = temp; // <- get rid of this
    cout << it->value << endl;
}

C++ vector std::bad_alloc error in a program

2 Answers2