2

I have many classes inherited from the abstract base SysLaserBase. I add every laser to a list in the program (vector of pointers to the objects) as you see in the function addLaser(). This works perfectly fine, and it's totally working, and I can cycle through all the lasers I created and use their methods with no problems.

The problem happens then when I want to "remove" a laser from the system. I do this by comparing the address of the object with all the addresses in the vector of the base class's pointers. Is this legal? it's causing the program to crash some times. What's the alternative?

Please find here the code of the methods, and then an example of the call that causes the crash. Do you find anything wrong in the code of removeLaser()? why does the whole system act crazy after the removal of a single laser?

the vector involved is in the base class's body:

std::vector<SysLaserBase<T>*> lasers;

And the add and remove methods:

template <typename T>
void SysSystemBase<T>::addLaser(const SysLaserBase<T> &src)
{
    bool alreadyThere = 0;
    for(unsigned long i = 0; i < lasers.size(); i++)
    {
        if(&src == lasers[i])
        {
            alreadyThere = 1;
            break;
        }
    }
    if(!alreadyThere)
    {
        lasers.push_back(const_cast<SysLaserBase<T>*>(&src));
    }
    signalsOut.resize(lasers.size());
}


template <typename T>
void SysSystemBase<T>::removeLaser(const SysLaserBase<T> &src)
{
    for(typename std::vector<SysLaserBase<T>*>::iterator it = lasers.begin(); it != lasers.end(); ++it)
    {
        if((*it) == &src)
        {
            lasers.erase(it);
        }
    }
    signalsOut.resize(lasers.size());
}

Call of this code:

sys.addLaser(las0);
sys.addLaser(las1);
sys.addLaser(las2);
sys.removeLaser(las0); //removes the laser, but cycling through the lasers through the vector of base class causes a crash
sys.removeLaser(las1);
sys.removeLaser(las2); //crashes the program immediately after this call

Thank you for any efforts :-)

Thank you for the replies. According to your replies I have changed removeLaser() to this:

template <typename T>
void SysSystemBase<T>::removeLaser(const SysLaserBase<T> &src)
{
    for(typename std::vector<SysLaserBase<T>*>::iterator it = lasers.begin(); it != lasers.end(); ++it)
    {
        if((*it) == &src)
        {
            lasers.erase(it);
            break;
        }
    }
    signalsOut.resize(lasers.size());
}

It's OK now. Thanks :-)

The Quantum Physicist
  • 24,987
  • 19
  • 103
  • 189
  • See: http://stackoverflow.com/questions/596162/can-you-remove-elements-from-a-stdlist-while-iterating-through-it – dbrank0 Apr 18 '12 at 13:15
  • Do you have a list for each inherited type? Did you know that you can keep a generic list and use dynamic_cast to grab the base class instance of a derived class? – std''OrgnlDave Apr 18 '12 at 13:19
  • Off-topic, but that `const_cast` is rather evil; either you should store `const` pointers, or `addLaser` should take a non-const reference. As it is, you're misleading the callers about whether it might be modified, and risking undefined behaviour if someone passes a reference to a `const` object. – Mike Seymour Apr 18 '12 at 13:24
  • Thanks for the advice about const_cast. I will remove it in my next update :-) – The Quantum Physicist Apr 18 '12 at 13:33
  • @OrgnlDave Not really. I don't have a list of inherited types. I need a single list for all the lasers in the system. Thanks for the idea. – The Quantum Physicist Apr 18 '12 at 13:34

1 Answers1

5

it is invalidated after the lasers.erase(). From std::vector::erase():

Iterators and references to the erased elements and to the elements between them and the end of the container are invalidated.

Either break from the for or save the return value from erase().

hmjd
  • 120,187
  • 20
  • 207
  • 252