-1

I have a vector that holds a pointer to classes:

vector<Entity*> editorElements;
vector<Entity*> entities;
vector<DirectionalLight*> dirLights;
vector<PointLight*> pointLights;
vector<SpotLight*> spotLights;

This code is within my class Scene. The Scene's ctor and destructors are like so:

Scene()
{
    editorElements = vector<Entity*>();
    entities = vector<Entity*>();
    dirLights = vector<DirectionalLight*>();
    pointLights = vector<PointLight*>();
    spotLights = vector<SpotLight*>();
}

~Scene()
{
    delete[] &entities;  // ERROR HERE
    delete[] &dirLights;
    delete[] &pointLights;
    delete[] &spotLights;
    delete[] &editorElements;
}

In the destructor I marked a line ERROR HERE. It doesn't matter which vector I put first, I always get the error. Strange is, it was working fine until lately (wasn't touching anything within the Scene class, or in any other classes that use an instance of Scene) and all of a sudden it raised an exception:

enter image description here

It doesn't matter if the vectors are empty or have elements, it gives the error all the same.

I require assistance to get this sorted out.

agiro
  • 2,018
  • 2
  • 30
  • 62
  • 2
    Where did you get the idea the you should use `delete` with vectors? You can't have seen any examples of it. – molbdnilo Sep 08 '18 at 11:29
  • 2
    All the assignments in you constructor are unnecessary. You're just replacing one empty vector with another. – molbdnilo Sep 08 '18 at 11:30
  • For the first - I have seen them using arrays and coming from a c#/java background, where Lists are like pumped arrays I thought it would work too. So far c++ didn't complain. Fir the second, I know, just for no apparent reason I wanted to make sure. – agiro Sep 08 '18 at 11:38
  • 5
    I would strongly advise trying to learn C++ properly rather than writing something like C# or Java and expecting it to work. Even if it compiles and _appears_ to work, you can get unpredictable Undefined Behaviour if you don't know what you're doing. Finally, if you do end up with something that happens to work, it will probably be very ugly and inefficient C++. – Useless Sep 08 '18 at 11:43
  • I know, that's why I started this whole rendering stuff in Java, using lwjgl. But that was terrible and I wasted a lot of time so I rather switched to c++. – agiro Sep 08 '18 at 11:45
  • 2
    You should take a look at the [book list](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). Despite the syntactic similarities, C++ is very different from the languages you're familiar with. – molbdnilo Sep 08 '18 at 12:01
  • @agiro *and coming from a c#/java background* -- To add what was stated, even *if* you got your program to work using Java-like coding, the program will probably be less efficient, full of hidden errors, and will just plain look weird to any seasoned C++ programmer. Things like using `new` all over the place when not necessary, creating "god" objects and deriving classes from it instead of using templates, etc. There are tell-tale markers that indicates the programmer failed to actually learn C++ and are using Java or C# as a model in writing C++ code. Don't fall into that trap. – PaulMcKenzie Sep 08 '18 at 12:30
  • 3
    You really can't learn `C++` using guesswork and compiler hints. You need to study it from a reliable text. – Galik Sep 08 '18 at 12:38
  • @PaulMcKenzie words to live by. The second part of your comment, especially the _new all over the place_, was that in general or specifically about my code? I use that only for objects I want to force to heap and will live until I destroy them myself or I stop the game's execution. I use pointers/refs when I need to modify an object's properties somewhere else so I can pass them around. I don't use `new` all over the code e.g. as local variables or passing matrices, vec3-s etc. – agiro Sep 08 '18 at 12:50

3 Answers3

2

The vectors aren't newed pointers, let alone newed arrays. So you shouldn't be deleting them. If you need to call delete on the pointers stored in the vectors, you should loop over the vectors, deleting each element. But you may be better off storing smart pointers instead (e.g. std::unique_ptr<Entity>. That is if you need to store pointers to dynamically allocated objects at all.

Note that if you do end up deleting elements in the destructor, you will also need to take care of the rule of three/five.

juanchopanza
  • 223,364
  • 34
  • 402
  • 480
  • I need to access an `Entity` or `Light` from multiple places within the code but still wish to keep track of them in one single place, but let others access the same instance (so no copying). Shall I use `shared_ptr` then? – agiro Sep 08 '18 at 11:51
  • @agiro It is probably better to see if `unique_ptr<>` satisfies your requirements first. That would fork, for example, if the `Scene` instance out-lives all the other things that need to access the entities and lights. – juanchopanza Sep 08 '18 at 11:53
  • It sure outlives, it is managed by `EngineInternal` and has only one instance created. Surface scripting can only use the one `Scene` object's functionality that was exposed (create new entity, create a light etc) – agiro Sep 08 '18 at 11:54
  • It's fine for the vector to store raw pointers so long as something else (presumably `EngineInternal`) _is_ taking care of the entity lifetimes. Ideally _that_ should be doing it using smart pointers, or by allocating them as a `vector`, or whatever. – Useless Sep 08 '18 at 12:05
2

The delete expression delete [] x is described thus:

Destroys an array created by a new[]-expression

So delete[] &entities only makes sense if &entities is an array created by a new[]-expression. Right?

But entities is a std::vector<Entity*>, not an Entity[]. You didn't create it with new[], so you must not delete it with delete[].

std::vector isn't syntactic sugar for an array, it's a template class, and std::vector<Entity*> entities isn't an array, it's an object with a constructor and destructor. That also tells us that this statement

entities = vector<Entity*>();

does nothing useful - entities is an object, so it was already default-constructed. You're just default constructing an anonymous temporary of the same type, and assigning it.

Finally, storing raw pointers in a vector is OK here as the vector isn't responsible for the objects' lifetimes. In most situations it's preferable to have the vector own the objects so you don't have to worry about deleting them manually, either directly with

vector<Entity>

or indirectly with

vector<unique_ptr<Entity>>

NB. a good guideline is: you shouldn't be using new, new[], delete or delete[] at all in user code.

Use a class object to manage your storage, because the compiler will take care of calling its constructors and destructors for you.

If you need a custom class, write one that only manages memory, so that stays decoupled from your program logic. Otherwise just use the standard library facilities like std::vector, std::array, std::unique_ptr, std::shared_ptr if you really need that ownership model, etc.

Useless
  • 64,155
  • 6
  • 88
  • 132
  • Thank you for the answer. Entity is like a "thing" within the game, with properties like a mesh, textures, transforms, shaders and the like. As a result I wanted to force them to heap and keep track of them in one single place - the scene, and delete the objects there when the game has finished. I'll use the smart pointer approach then. – agiro Sep 08 '18 at 11:41
  • "storing raw pointers in a vector is frequently a bad idea, because..." Not all raw pointers need to be deleted. – juanchopanza Sep 08 '18 at 11:41
  • "storing raw pointers in a vector ... if the vector is supposed to own them." But yeah, I can probably clarify: that sentence came out as badly as Java translated into C++. – Useless Sep 08 '18 at 11:45
1

all of a sudden it raised an exception

It is because of trying to delete something you shouldn't be deleting. The delete[] syntax is for deletion of a dynamically allocated array. But you provide it with a pointer to a std::vector instance. So the compiler takes this address for the deletion as if it was an array, which includes finding out its size and then delete the entire segment. But there's no such appropriate figure, as this is not an array, so in run-time you end up trying to delete something in a place you don't have permission to access and thus you get the assertion failure, as this is an access violation, a.k.a. segfault.

Also, vector is a class that manages its own memory. If you wanted to release entities held inside this container -- that is, the individual elements themselves, allocated dynamically -- then you should loop over them and delete each one. Conveniently, for example, using auto and a range-based for loop like this:

for (auto ptr : entities)
    delete ptr;

But, in most scenarios you better save yourself this memory management overhead and opt for std::unique_ptr instead of the raw pointers:

#include <memory>
...
std::vector<std::unique_ptr<Entity>> entities;

This way you don't have to worry about freeing any memory as it will be released by the std:: unique_ptr once it's being destructed, as part of the vector's destruction.

Further, this is unnecessary and probably not what you intended to do:

entities = vector<Entity*>();

as the vector object itself is already defined (so it is existing) before this line, and all it does is create an identical new one and assign it to entities.

Geezer
  • 5,600
  • 18
  • 31