2

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?

Improve this question
13
  • 1
    Since each token will be registered/unregistered in that list during the Token lifetime you might end up deleting tokens defined on the Stack or inside another class. Also , how to you create the tokens ? Commented Mar 10, 2014 at 15:28
  • 1
    Hm, the Token::DeleteRegistered() won't delete most of the objects, but I don't see how that would cause a crash. Are you perhaps using the id member for anything? Once you delete an object, the ids stop matching and you will be getting multiple instances with same id... Commented Mar 10, 2014 at 15:30
  • 3
    I don't know whether it's causing your problem, but you'll need a virtual destructor. There's also no way to ensure that all instances are created with new; how are you making sure all of them are deletable? Commented Mar 10, 2014 at 15:31
  • 1
    In order to avoid deleting objects twice or more, I store references to all of the allocated instances, Maybe you should invest in using a smart pointer such as std::shared_ptr instead of trying to keep track of number of instances. Using shared_ptr, once remove() does its job, that's it. You don't need that static member array. Commented Mar 10, 2014 at 15:36
  • 1
    @Dennis: "Can you delete stack allocated objects?" - No, you can't. This design requires everything to be created with new; which is one possible cause of failure. Commented Mar 10, 2014 at 15:49

2 Answers 2

Reset to default
8

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

18 Comments

The iteration was with indices, not iterators, so the only thing it did was it failed to delete half of the objects. But I don't see how it could have caused a crash.
I am afraid that with your suggestion, the error still shows up.
@fasked: But the destructor (called by the delete expression) does.
@fasked you are incorrect in this case, because the Token destructor changes the size of the vector
ok. I see your edit. Seriously, your issue here would be greatly reduced, if not totally disappear if you used a shared_ptr or shared_container.
|
2 
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();
    }
}
Improve this answer

5 Comments

remove isn't going to modify registered (at least, not in any way that invalidates iterators), so it doesn't matter which order the arguments of erase are evaluated in.
@MikeSeymour - I stand corrected. I'm deleting (and erasing!) the first part of my answer. The problem is in DeleteRegistered, not Token::~Token.
@DavidHammen Trying out your solution, it does manage to delete the first two Token's, but crashes afterwards.
@Tyymo - As PaulMcKenzie suggested a couple of minutes ago in his comment to your question, you should show us a minimal working example.
Despite the fact that your suggestion did not work, it led me to find the true problem with my code. Thanks

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.