Populating a vector from function

Question

I am trying to do something like this but I'm not sure what is the best way. What am I doing wrong? I have also tried changing double v[] to double *v

#include <iostream>
#include <random>
using namespace std;
void PopulateVector(double v[])
{
    delete v;
    v = new double[5];
    v[0] = 1;
    v[1] = 1;
    v[2] = 1;
    v[3] = 1;
    v[4] = 2;

}

int main()
{
    double *test = new double[1];
    PopulateVector(test);
    for (int i = 0; i < 5; i++)
    {
        cout << test[i] << endl;
    }
}

Based on the good comments. I made some modifications. This version works but i still wish the void PopulateVector(double *v) or PopulateVector(double v[]) worked.

#include <iostream>
#include <random>
using namespace std;
double* PopulateVector()
{
    double *v = new double[5];
    v[0] = 1;
    v[1] = 1;
    v[2] = 1;
    v[3] = 1;
    v[4] = 2;
    return v;
}

int main()
{
    double *test = new double[1]; 
    delete[]test; // Do I need this?
    test = PopulateVector();

    double *test2 = new double[1];
    test2 = PopulateVector();
    for (int i = 0; i < 5; i++)
    {
        cout << test[i] << endl;
    }
}

"What am I doing wrong?" Don't use `delete` for something that was `new[]`. Moreso, use `std::vector` instead of manually managing the container. — Eljay, Mar 26 '19 at 02:44
Thanks but I'm trying to avoid using std::vector. I have gotten it to work with that but I'm trying to test the speed of a code using std::vector and without. — Pat, Mar 26 '19 at 02:46
What are you trying to do? Also, why are you trying to avoid `std::vector`? — , Mar 26 '19 at 02:47
See this: https://stackoverflow.com/questions/2425728/delete-vs-delete-operators-in-c — Thomas Jager, Mar 26 '19 at 02:47
I simplified some of my code to load giant matlab data files into c++. Basically I want to load large vectors and perform operations on them. I may or may not know the size. I found that with std::vector c++ was more than 10x slower than doing the operation within matlab so I am trying without. — Pat, Mar 26 '19 at 02:48
It crashes on my machine at `delete v;` because the `v` was constructed with `new[]`. The code as given is undefined behavior. — Eljay, Mar 26 '19 at 02:48
What is the purpose of allocating memory before calling the function, if you just plan to discard it? Why not define your function as `double *PopulateVector()`? — paddy, Mar 26 '19 at 02:51
Just don't use `new` or `delete`. They're almost never necessary any more, because there are standard classes covering all their common use cases, and making things much easier on you. In this case, you want a `std::vector`, or possibly a `std::array`. — aschepler, Mar 26 '19 at 03:01

Dmytro Dadyka · Accepted Answer · 2019-03-26T03:39:18.197

3

What am I doing wrong?

You use delete for array.
You assign value to local copy of v variable.
You violate the principle of "destruction in the same place where you create".
Do not free memory.
You don't return integer from int main

My advice to you is to use stl!

what is the best way.

It's a matter of taste, but I would do something like that:

#include <iostream>
#include <random>
#include <vector>

std::vector<double> PopulateVector()
{
   std::vector<double> v(5);
   v[0] = 1;
   v[1] = 1;
   v[2] = 1;
   v[3] = 1;
   v[4] = 2;
   return v;
}

int main()
{
   std::vector<double> test = PopulateVector();
   for (int i = 0; i < test.size(); i++)
   {
      std::cout << test[i] << std::endl;
   }
   return 0;
}

edited Mar 26 '19 at 03:39

answered Mar 26 '19 at 02:59

Dmytro Dadyka

2,208
5
18
31

You are right. This is the best way. I think I'll combine your response with this one to extract my data https://stackoverflow.com/questions/2923272/how-to-convert-vector-to-array – Pat Mar 26 '19 at 03:03
Under the hood, `std::vector` is doing all the hard work that you are attempting to do here, but it does it in an exception-safe way without any memory leaks. There is no speed tradeoff for your application (*** note that this answer _should_ have called `v.reserve(5)` prior to pushing values). There are almost _NO_ situations for you where it will be appropriate (for you) to use _raw_ pointers, until (maybe) you are sufficiently advanced as a software engineer. And at that stage, you won't be asking about it in the way that you approached this question. – paddy Mar 26 '19 at 03:10
1

Just a comment on style for this answer. Passing the vector as a reference to a function called `PopulateVector` is misleading. _Should_ the vector be empty when calling? _Should_ memory already be reserved by the caller so that _push_back_ doesn't potentially result in multiple allocations? _Should_ the function call `clear`, in case the vector already has elements, or is it intended to append to existing data? These are the kinds of problems with using that style. It is generally better for the function to _return_ the vector instead of taking it as an argument. – paddy Mar 26 '19 at 03:14
@paddy, agree with you. The name of the function suggests that the function fills the new array, rather than adding elements to the existing one. Yes, the signature should eliminate ambiguity as much as possible. – Dmytro Dadyka Mar 26 '19 at 03:30
1

Interesting fun fact. `main` is the one function with a non-`void` return type where you don't have to return anything. 0 is implied if return is omitted. – user4581301 Mar 26 '19 at 03:40

score 1 · Answer 2 · answered Mar 26 '19 at 03:16

1

To answer your updated question, PopulateVector(double *v) can be made to work as long as the array is allocated by the caller and not the function itself

void PopulateVector(double * v)
{
    v[0] = 1;
    v[1] = 1;
    v[2] = 1;
    v[3] = 1;
    v[4] = 2;
}

int main()
{
    // Caller allocates the memory
    double * test = new double[5];
    PopulateVector(test);
    delete[] test;

    // Or even better, keep it on the stack
    double test2[5];
    PopulateVector(test2);
}

answered Mar 26 '19 at 03:16

Peter Bell

371
2
6

This assumes that the caller knows the function will write at least 5 values, and that the function knows the caller will allocate memory for at least 5 values. Design like this is problematic. Maybe not so much in a contrived example like this, although this is exactly where bad habits are nurtured. As such, I would not personally recommend either of the suggestions offered by this answer. – paddy Mar 26 '19 at 03:23
For a general use case, yes this is bad idea. Case in point `sprintf` being almost impossible to use correctly. – Peter Bell Mar 26 '19 at 03:31
Huh... `sprintf` is easy to use correctly, and using that as an example is completely missing the point of what I'm saying. Unless you don't realize that `sprintf` can _tell you_ precisely how many bytes it will write, if you pass NULL, and in most cases the caller _knows_ based on the format specifier how much buffer they need. – paddy Mar 26 '19 at 03:38
Worked mostly in C for 20 years and I think my success rate with `sprintf` has to be over 50%. Didn't take much convincing to get me to switch to `snprintf`, though. – user4581301 Mar 26 '19 at 03:44
My choice of words was poor. I simply meant that `sprintf` encourages people to just pass a buffer they _hope_ is big enough, creating an additional source of bugs. Not only is it easier to write this way, but it's more efficient since it only requires a single pass instead of two. Whether you like `sprintf` or not, I think it has an interface that is easy to misuse. However, these types of interface still make sense when the caller knows the correct size a-priori, e.g. because it's a private member function in the same class. – Peter Bell Mar 26 '19 at 03:51

Populating a vector from function

2 Answers2