C++ - Cannot return vector from function

Question

Assume the following function

std::vector<double> LocatePulseEdges(int points, double* signal_x, double* signal_y, double threshold, vector<double> left_edge, vector<double> right_edge){
cout << "1" << endl;
    for (int i=0; i<points; i++){
        if(signal_y[i]<threshold){// left side of the pulse
            left_edge.push_back(signal_x[i]);
            break;
        }
        if(signal_y[i]>threshold){// right side of the pulse
            right_edge.push_back(signal_x[i]);
            break;
        }
    }
cout << "6" << endl;
    return left_edge;
    //return right_edge;

cout << "7" << endl;
}

I am calling this function in the following code

void Analyze(){
    int points = 90000000;//hSignal->GetNbinsX();
    double* x          = new double[points];           // SIZE limited only by OS/Hardware
    double* y          = new double[points];
    std::vector<double> left;
    std::vector<double> right;
    double threshold = 6234.34;

    Function_to_Fill_X_and_Y();
    LocatePulseEdges(points, x, y, threshold, left, right);
    cout << "First left threshold crossing @ time : " << left[0] << endl;
}

Although I get no compilation error, when I run the program, it crashes right before the return statement.

Any idea on why this is happening?

@Boiethios : Hmmm... How can I check that? And if this the case, how can I solve it? — Thanos
– Thanos, Commented Jun 24, 2016 at 8:34
@GMichael : What else can I do? I am not that familiar with pointers... :( — Thanos
– Thanos, Commented Jun 24, 2016 at 8:37

PaulMcKenzie · Accepted Answer · 2016-06-24 10:10:33Z

There are several flaws in the LocatePulseEdges function, as well as the Analyze function.

First, if you're going to use std::vector<double> in one section of the code, why not use it throughout? You have:

void Analyze()
{
  //...        
  double* x = new double[points]; 
  double* y = new double[points];
 //...
}

Unless you've called delete [] x and delete [] y, this function has a memory leak. You could have simply used

std::vector<double> x(points), y(points);

and in the function to fill them, pass x.data() and y.data() if using C++11, or if not, &x[0] and &y[0]. This alleviates the memory leak.

Even if you did have delete [] somewhere, if an exception is thrown, that delete [] may be bypassed, causing a leak. Using std::vector, the vector is destroyed even on exception.

Second, for the LocatePulseEdges function, pass the vector's by (const) reference, not by value. In addition, there is no need to return the vector by value. If you were creating a brand new vector within the function, then possibly that would justify the returning of the new vector, but you aren't doing that. So return a void.

void LocatePulseEdges(int points, double* signal_x, double* signal_y, double threshold, vector<double>& left_edge, vector<double>& right_edge)
{
    //...
}

When you passed the vector by value, a copy of the vector is made, so you're left_edge.push_back() call was working with a temporary, not with the actual vector you were passing. That's why on return, the vector left_edge was empty.

Last, check for vector::empty() if you're going to access the first item in the vector. You cannot just assume that the item exists in the vector.

   LocatePulseEdges(points, x.data(), y.data(), threshold, left, right);
   if ( !left.empty() ) 
       cout << "First left threshold crossing @ time : " << left[0] << endl;
   else
       cout << "No threshold's generated" << endl;

The code above assumes you took the advice of using std::vector<double> for the x and y variables.

Irminsul · Accepted Answer · 2016-06-24 08:47:34Z

Pass your left_edge by reference so it can be modified by your fonction :

void LocatePulseEdges((int points, double* signal_x, double* signal_y, double threshold, 
std::vector<double> &left_edge, std::vector<double> &right_edge)
{
    //do your stuff
}

when you give argument without the &, the programm copies the value of your argument and then use it. So you can't modify it. Passing argument with the & is called pass by reference.

When you pass an object by reference, you can modify it, and it is faster. If you need to pass an object without modify it, give a const ref :

void foo(const std::vector<double> &vec)

is faster than

void foo(std::vector<double> vec)

and prevents you to modify vec thanks to the const keyword.

Additionnal note :

a void function does not require any return, but in many cases its better to do a Bool function

Bool LocatePulseEdges()
{
    //do your stuff
    return True ;
}

Making it this way allows you to return False prematurly if anything goes wrong.

MaciekGrynda · Accepted Answer · 2016-06-24 09:07:54Z

1

Basing on comments I get that:

Change LocatePulseEdges function to use reference to:

void LocatePulseEdges((int points, double* signal_x, double* signal_y, double threshold, std::vector<double> &left_edge, std::vector<double> &right_edge) { //do your stuff } So you can change value of arguments inside of function
Check if element exists in vector before you access it:

if (left.size() <= index) { return left[index]; }
Now what's wrong with your code. for (int i=0; i<points; i++) Is executed, because points = 9000000, but signal_y[i]<threshold is never true (it's not initialized nowhere in your code), so nothing is inserted to left.

answered Jun 24, 2016 at 9:07

MaciekGrynda

7033 silver badges13 bronze badges

Collectives™ on Stack Overflow

C++ - Cannot return vector from function

3 Answers 3

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related