Is this a safe way to free memory?

Question

I have a member variable called neuronLayers which is of type std::vector<Eigen::RowVectorXf*> and another called weights of type std::vector<Eigen::MatrixXf*>.

As part of a forward propagation function, I need to multiply values from these two vectors and keep a pointer to that value. I've already made sure the sizes of the dimensions of values I'm multiplying match, so there's no concern there.

I came up with this:

for (unsigned i = 1; i < this->topology.size(); i++) {
    delete this->neuronLayers[i];
    this->neuronLayers[i] = new Eigen::RowVectorXf((*this->neuronLayers[i - 1]) * (*this->weights[i - 1]));
}

Unfortunately, memory management is a weak point of mine, so I might be missing something blindingly obvious. I'm just conscious that carelessly creating new objects can lead to memory leaks.

When I ask if this is safe, what I want to know is if this could cause to memory leak, or somehow lead to some other memory error? If it could, how could I modify that code snippet to fix this issue?

Edit: neuronLayers is used elsewhere with other code that expects it to be a vector of Eigen::RowVectorXf* pointers rather than a vector of Eigen::RowVectorXf objects. The snippet shown doesn't show this.

Why use pointers at all? Use `std::vector` and `std::vector` instead, then you won't have anything to `new` or `delete`. But, if you must, then consider using `std::unique_ptr` instead of raw pointers, then you don't have to worry about `delete` manually. And you can use `std::make_unique()` to avoid `new`. — Remy Lebeau, Jul 20 '21 at 17:16
Why are you even using `new` and `delete` to begin with?! Can't you do `std::vector`, and so on? — HolyBlackCat, Jul 20 '21 at 17:17
If you cannot hold these objects by value, at least use `shared_ptr` — Igor R., Jul 20 '21 at 17:18
Related: [https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new) — drescherjm, Jul 20 '21 at 17:18
Note: `std::shared_ptr` has not-insignificant overhead to manage the sharing and is best reserved for cases where the object legitimately has two or more owners with a stake in the lifetime of the object. If this isn't the case for your code, use `std::unique_ptr` and have one owner. — user4581301, Jul 20 '21 at 17:24
Is there some reason you need to create a new object rather than change the value of the existing object? — David Schwartz, Jul 20 '21 at 18:40
Your code appears not to delete the last element in the vectors. Why do you loop from `i=1` instead of `i=0` and index `[i-1]` instead of `[i]`? — doug, Jul 20 '21 at 20:45
@doug the last element is deleted. I'm deleting element i rather than i-1. Element 0 is skipped here, but that's a special case so it gets dealt with before this loop starts. — Chukwuemeka Chukwuenweniwe, Jul 22 '21 at 16:54

Remy Lebeau · Answer 1 · 2021-07-20T18:37:29.667

8

A better option would be to not use any dynamic allocations at all. You don't need new and delete in this case, just get rid of the pointers and hold actual objects in your vectors instead, eg:

std::vector<Eigen::RowVectorXf> neuronLayers;
std::vector<Eigen::MatrixXf> weights;
...
for (unsigned i = 1; i < topology.size(); i++) {
    neuronLayers[i] = Eigen::RowVectorXf(neuronLayers[i - 1] * weights[i - 1]);
}

edited Jul 20 '21 at 18:37

answered Jul 20 '21 at 17:21

Remy Lebeau

555,201
31
458
770

If I had written the whole thing from scratch, I probably wouldn't have used pointers. But other parts of the code that I did not write expect a vector of pointers. Trying to go along with that code seemed like a better idea than trying to replace it. – Chukwuemeka Chukwuenweniwe Jul 20 '21 at 18:04
1

@ChukwuemekaChukwuenweniwe Then you'll need to figure out *why* that code uses a vector of points. It may be, for example, that multiple pointers to the object are kept and, in that case, just deleting it and allocating a new object is asking for trouble. – David Schwartz Jul 20 '21 at 18:39
@DavidSchwartz That's a good point. In this case, I don't think it will be an issue, but I wasn't thinking about that before. Thanks for pointing it out. – Chukwuemeka Chukwuenweniwe Jul 22 '21 at 16:57

Is this a safe way to free memory?

1 Answers1