tags:

views:

3683

answers:

3

Hi,

I have an abstract base class called Shape from which both Circle and Rectangle are derived, but when I execute the following code in VS 2005 I get the error Debug assertion failed. At the same time I have not overloaded == operator in any class

Expression:Vector iterator not dereferencable, what is the reason for this.

  vector<Shape*> s1;
  s1.push_back(new Circle(point(1,2),3));
  s1.push_back(new Circle(point(4,3),5));
  s1.push_back(new Rectangle(point(1,1),4,5));

  vector<Shape*> s2(s1);
  reverse(s1.begin(),s1.end());

  (*find(s1.begin(),s1.end(),new Circle(point(1,2),3)))->move(point(10,20));
+6  A: 

Simple :

  • find fails since your newly created Circle can't be found in the vector with comparing Shape *
  • a failed find returns the end iterator which is not deferencable as caught by a Debug assertion

For it to work like you want, you do need to compare Shape, not Shape*

As pointed out in other answers, boost::ptr_vector is an easy way to achieve this.

David Pierre
And to compare Shape objects rather than Shape Pointer use boost::ptr_vector. This allows you to use the normal algorithms much more naturally.
Martin York
+4  A: 

Like @David Pierre suggests: find is value-based: it looks in the range of iterators for a pointer (e.g. 0x0F234420) that equals the pointer to the new Circle(point(1,2),3) you just created. Since that's a new object, it won't be there.

You can get around this by using find_if with an operator that compares the objects referenced to by the pointer.

However, the Criterium should be able to differentiate between shape types.

class Shape {
public:
    //amongst other functions
    virtual bool equal( const Shape* ) const = 0;
};

class Circle : public Shape {
public:
    bool equal( const Shape* pOther ) const {
        const Circle* pOtherCircle = dynamic_cast<const Circle*>( pOther );
        if( pOtherCircle == NULL ) return false;
        // compare circle members
    }
};

class Rectangle : public Shape {
public:
    bool equal( const Shape* pOther ) const {
        const Rectangle* pOtherR = dynamic_cast<const Rectangle*>( pOther );
        if( pOtherR == NULL ) return false;
        // compare rectangle members
    }
};



Shape* pFindThis = new Circle(point(1,2),3);
vector<Shape*>::const_iterator itFound = find_if(s1.begin(),s1.end(), 
    bind1st( mem_fun( &Shape::equal ), pFindThis) ) );
delete pFindThis; //leak resolved by Mark Ransom - tx!

if( itFound != s1.end() ) {
    (*itFound)->move(point(10,20));
}
xtofl
Does bind1st delete the second parameter when it's done? If not, you have a memory leak. I think you just want a local temporary there.
Mark Ransom
You're absolutely right.
xtofl
+2  A: 

This is a good reason to use boost::ptr_vector.

It not only handles the fact that your objects need to be destroyed.
xtofl@: You forgot the virtual destructor.

But it also makes the members look like objects by returning references rather than pointers. This allows you to use the standard algorithms much more naturally rather than playing around with pointers in your 'equal' function (which is very un C++ like).

#include <boost/ptr_container/ptr_vector.hpp>
#include <iostream>

class Shape
{
    public:
        ~Shape()    {}
        bool operator==(Shape const& rhs) const
        {
            if (typeid(*this) != typeid(rhs))
            {
                return false;
            }

            return this->isEqual(rhs);
        }
    private:
        virtual bool isEqual(Shape const& rhs) const    = 0;
};

class Circle: public Shape
{
    public:
        Circle(int r)
            :radius(r)
        {}
    private:
        virtual bool isEqual(Shape const& r) const
        {
            Circle const&   rhs = dynamic_cast<Circle const&>(r);
            return radius == rhs.radius;
        }
        int radius;
};
class Rectangle: public Shape
{
    public:
        Rectangle(int h,int w)
            :height(h)
            ,width(w)
        {}
    private:
        virtual bool isEqual(Shape const& r) const
        {
            Rectangle   const&  rhs = dynamic_cast<Rectangle const&>(r);
             return (height == rhs.height) && (width == rhs.width);
        }
        int height;
        int width;
};


int main()
{

    boost::ptr_vector<Shape>    data;

    data.push_back(new Circle(5));
    data.push_back(new Circle(6));
    data.push_back(new Rectangle(7,4));

    boost::ptr_vector<Shape>::iterator f;
    f = find(data.begin(),data.end(),Circle(6));

    std::cout << "Find(" << (f - data.begin() ) << ")" << std::endl;


}
Martin York
It seems a little dangerous to rely on operator== to validate that the shapes are the same type before calling isEqual. I'd prefer to see that check inside isEqual itself, as xtofl did.
Mark Ransom
I prefer it in 'operator==' this way the code is not repeated. Why would you want it in isEqual? Note Shape is a pure virtual there can be no instances. I don't see why you think it is dangrorus?
Martin York
I have one doubt regarding checking typeids in base classes. Why this is actually needed ? Because call to correct isEqual function is resolved at runtime because of Polymorphism.
siri
sirishkumar.myopenid.com@ It is probably an early optimization I admit that. But if you don't check in 'operator==' then every version of isEqual() also needs to explicitly validate that the type 'rhs' parameter, requiring dynamic_cast<>() and a check for bad_cast exception or NULL.
Martin York