0

I've been working on a Qt project and I encountered a problem with deleting objects that are being hold in a map. I prepared a simple C++ code to show my problem:

   #include <iostream>
#include <map>
#include <string>

using namespace std;

    class A 
    {
    public:
        int *tab;

        A()
        {
            tab = NULL;
        }
        ~A()
        {
            if (tab != NULL)
            {
                delete[] tab;
            }
        }
    };

        int main()
        {
            map<string, A> mapa;

            string name = "MyArray";

            A *a = new A;
            a->tab = new int[3];
            a->tab[0] = 1;
            a->tab[1] = 2;
            a->tab[2] = 3;

            mapa[name] = *a;

            delete a;


            system("PAUSE");
            return 0;
        }

After closing the program I get: Debug Assertion Failed!

_BLOCK_TYPE_IS_VALID etc..

My question is: why is that? The reason is probably that map gets deleted after I quit the program and it holds an A object (a) that is being deleted before I close the program. However, I passed a value, not an address, so what's the problem?

Isn't that value just copied into the map and held in some different address?

MindRoller
  • 241
  • 4
  • 15
  • 1
    See http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – juanchopanza May 05 '16 at 09:31
  • I'm OT, but in the spirit of pointing you to the rule of three... I suggest you read the "Effective C++" series by Scott Meyers. – StoryTeller - Unslander Monica May 05 '16 at 09:33
  • 1
    @n.m.: I already said that. In the answer section. Your comment adds nothing... – Lightness Races in Orbit May 05 '16 at 09:38
  • See also: [Rule of Five](http://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c11) and [Rule of Zero](https://rmf.io/cxx11/rule-of-zero/). – n. m. could be an AI May 05 '16 at 09:41
  • @Lightness yes you did, but it is not clear from your answer what exactly the OP should do. Too many bits of advice to follow. My comment adds some certainty. But I can remove it if you insist ;) – n. m. could be an AI May 05 '16 at 09:46
  • _"`A` should have a `std::vector` instead."_ seems pretty clear to me, @n.m! – Lightness Races in Orbit May 05 '16 at 09:53
  • @Lightness it may be clear to you (and to me), but why is OP talking about fixing the problem with a copy constructor? Perhaps it was not sufficiently clear in the end? Mind you, I'm asking because I think it's *important*. Don't say "you need a copy ctor and an assignment here, and oh by the way use std::vector instead of all this", because they will follow the first advice, which is the wrong thing to do. – n. m. could be an AI May 05 '16 at 10:01
  • @n.m.: I try not to treat question authors like children (unless they're abusing tags :P). I have presented two options; it's now the OP's decision as to which to use, not mine or yours. FWIW, the OP seems pretty content with the information provided. Do you have any evidence to suggest that there is confusion here? – Lightness Races in Orbit May 05 '16 at 10:10
  • @LightnessRacesinOrbit Obviously OP needs *guidance* (not all question authors need guidance, but some do). You have presented two options, both of them equally new and exciting, without as much as a hint of criteria to choose between them. What do you expect them to do? Start studying the subject on their own? Maybe we should just point to a good C++ book and say "all your options are here, study this book now and you shall be enlightened". Will save lots of typing when answering questions ;) – n. m. could be an AI May 05 '16 at 10:23
  • @n.m.: _"What do you expect them to do? Start studying the subject on their own?"_ YES!!! I will never understand the view some people have that we're in some way a substitute for reading/thinking/learning/studying. – Lightness Races in Orbit May 05 '16 at 10:34
  • Guys no point of keeping this discussion going, I read 2 books of C++ I just wrote a proper constructor and overloaded a = operator and it didn't work. I found (I don't know, a "bug?" don't know how to call it) something interesting in Qt - I've been using QMap to store elements. And even though copy constructor was there, QMap was causing problems and program was crashing when I tried to add element to map. I changed QMap for normal C++ map and everything is OK. Perhaps QMap is designed to store instances of Qt classes, not instances of normal C++ classes. Thanks once again. – MindRoller May 05 '16 at 10:57
  • Also, I don't understand a saying that I should use a std::vector, while using normal 2d arrays can be just as good if you allocate everything well. Using std::vector is easier of course, however as you can see the problem was STILL elsewhere and not there. – MindRoller May 05 '16 at 10:59
  • Just use std::vector. It already allocates everything well for you. There is no need to reinvent the wheel. If your class works with std::map and doesn't work with QMap, it doesn't mean QMap is bad and your class is good and allocates everything correctly. Perhaps std::map just lets you get away with your errors (for now). – n. m. could be an AI May 05 '16 at 12:09
  • And no, QMap is not designed to store only instances of Qt classes. – n. m. could be an AI May 05 '16 at 12:17

1 Answers1

4

Yes, you're double-deleting.

Maps copy their elements, and your type has no copy constructor. You're actually copying the A yourself with mapa[name] = *a anyway.

The ownership of the member pointer is also very unclear.

Abide by the rule of three. Ideally avoid the manual dynamic allocation altogether; A should have a std::vector<int> instead.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • Well, as I said, I've been working on a Qt project and there was no way to avoid dynamic allocation, I just had to use it. Well, thanks for the answer, so actually, if a type has no copy constructor, a map will hold a pointer to that object. A copy constructor should fix that problem, well, I'm stupid :s – MindRoller May 05 '16 at 09:32
  • @MindRoller: If a type has no copy constructor written by you, then it has a copy constructor generated by the compiler which simply copies the pointer. Then you have two `A`s "owning" the same pointer, and both will `delete[]` it on destruction. You'd need to implement a deep copy or remove the `delete[]` from the destructor. As I said, the ownership of that dynamically-allocated memory is not clear in your program. – Lightness Races in Orbit May 05 '16 at 09:33
  • 1
    Alright, thanks, I will remember it for the rest of my life, lol. – MindRoller May 05 '16 at 09:36