CODEWITHSUNDEEP
c++memory-management
Novak
Novak

Reputation: 2768

C++ Unexpected error when deleting objects

I write an interpreter, in which each keyword, syntax notation or operator has the base class of Token.

class Token {
    private:
        static std::vector<Token *> registered;

        size_t id;

        std::string name;
        std::string symbol;

    public:
        Token(const std::string& Name, const std::string& Symbol);
        Token::~Token();

        Token(const Token& rhs) = delete;
        Token& operator =(const Token& rhs) = delete;

        /* ... */

        static void DeleteRegistered();
};

The constructor:

Token::Token(const std::string& Name, const std::string& Symbol)
                : name(Name), symbol(Symbol) {
    Token::registered.push_back(this);
    this->id = Token::registered.size();
}

The destructor:

Token::~Token() {
    // Removes 'this' from Token::registered
    Token::registered.erase(std::remove(Token::registered.begin(), Token::registered.end(), this), Token::registered.end());
}

DeleteRegistered:

void Token::DeleteRegistered() {
    for (size_t i = 0; i < Token::registered.size(); ++i) {
        delete Token::registered[i];
    }
}

In my code, many different classes store containers of pointers to sub-classes which eventually derive from Token.

In order to avoid deleting objects twice or more, I store references to all of the allocated instances, and have a static method which will delete them all.

The method DeleteRegistered is called after all operations are done executing.

Now, to my problem:

When I call Token::DeleteRegistered (which happens a few lines before the program exits, it fails and in debug shows the following:

File: f:\dd\vctools\crt\crtw32\misc\dbgdel.cpp
Line: 52

Expression: _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)

The memory, just one line before the programs crashes

Since all of the Token instances don't really have a defined scope, I came up with this design, which to me currently seems OK.

What can cause this error?

EDIT: The destructor was a late addition of mine, commenting it out still shows the error above. The delete fails to delete even the first item in the container.

2ND EDIT: An example of how I use Token:

this->parser.Operators.Add(new RefBinaryOperator(
    "Assignment", "=", 14, RefBinaryOperator::Assign
));

Note: RefBinaryOperator is a sub-class of Token (Not direct), which eventually calls Token's constructor.

So for instance, I pull pointers to Tokens from the Operators container, and assign them to other structures. When everything is done, I call DeleteRegistered.

FINAL EDIT:

I got it working by declaring the Token destructor as virtual:

Does delete work with pointers to base class?

Upvotes: 2

Views: 1133

Answers (2)

David Hammen
David Hammen

Reputation: 33106

void Token::DeleteRegistered() {
    for (size_t i = 0; i < Token::registered.size(); ++i) {
        delete Token::registered[i];
    }
}

The above will not do what you want. It's going to remove the first (0th) element from registered. What was the second element now becomes the first (0th element), but now i will be 1. Your loop will remove only every other element from Token::registered. It leaks.

One way to handle this: Keep removing either the first or last element while the vector is not empty. I'd suggest deleting the last element because that's more consistent with how vectors work. Deleting the first element until the vector is empty involves rebuilding the vector each step.

void Token::DeleteRegistered() {
    while (! Token::registered.empty()) {
        delete Token::registered.back();
    }
}

Upvotes: 2

pippin1289
pippin1289

Reputation: 4939

What is happening is that in Token::~Token() you are calling erase from the registered vector which is invalidating the indices used in the for loop where you noticed the problem. You need to remember that once you call erase on that vector, the for loop indices need to be adjusted properly. If you keep your current destructor the following could work in DeleteRegistered:

void DeleteRegistered() {
  while(!Token::registered.empty())
    delete *Token::registered.begin();
}

Also, since you have many classes which extend off of Token, ~Token() should become virtual so that the destruction for the base class will be properly handled.

Upvotes: 8

Related Questions