QList<mypointer *>::contains and operator== issue

[jamo_]

Hi,

I'm having some issues with QList<mypointer *>::contains and operator==.

It is probably easiest to explain this with code - am basically attempting to compare a pointer with one in a list.

QList <MyPointerBase *> p1;
QList <MyPointerInherited *> p2; //already instantiated
 
	for (int i = 0 ; i < p2.size() ; i++) {
		MyPointer *mp = new MyPointerBase(p2[i]);
		if (p1.isEmpty() || !p1.contains(mp))
		{
			p1 << mp;
		}
	 }

QList <MyPointerBase *> p1;
QList <MyPointerInherited *> p2; //already instantiated

	for (int i = 0 ; i < p2.size() ; i++) {
		MyPointer *mp = new MyPointerBase(p2[i]);
		if (p1.isEmpty() || !p1.contains(mp))
		{
			p1 << mp;
		}
	 }

To copy to clipboard, switch view to plain text mode 

However the !p1.contains(mp) always returns true, I suppose because it is comparing two pointers?

I have overloaded operator== for the two classes as well using both methods

Surely the second one should be used for the comparison? I have dbugged this but the code never makes it here..

bool operator== (MyPointerBase &pb1, MyPointerBase &pb2) {
	return (pb1.id == pb2.id);
}
 
bool MyPointerBase::operator== (const MyPointerBase *other) const {
	return (this->id == other->id);
}

bool operator== (MyPointerBase &pb1, MyPointerBase &pb2) {
	return (pb1.id == pb2.id);
}

bool MyPointerBase::operator== (const MyPointerBase *other) const {
	return (this->id == other->id);
}

To copy to clipboard, switch view to plain text mode 

Aside from creating my own version of "contains" can anyone see anything obvious I am doing wrong?

Thanks

:-)

[minirop]

If I not mistaken, bool MyPointerBase:perator== (const MyPointerBase *other) const { is never called because you compare pointers.

edit : and operator== takes (or should take) only references. wiki link

[john_god]

What a mess you've got there
If I understood it correctly, you want to check all elements from p2 and if they are not already in p1, you want to append them ?

for(int i=0;i<p2.size();i++)
    {
        if (!p1.contains(p2.at(i) )
        {
            MyPointerBase *mp = new MyPointerBase ();
            *mp=*p2.at(i);
            p1.append(mp);
        }
    }

for(int i=0;i<p2.size();i++)
    {
        if (!p1.contains(p2.at(i) )
        {
            MyPointerBase *mp = new MyPointerBase ();
            *mp=*p2.at(i);
            p1.append(mp);
        }
    }

To copy to clipboard, switch view to plain text mode 

I have dbugged this but the code never makes it here..

Because in your code you not using the operator == (at least in the code you show).
I have not tested my code, dont now if it will work

[jamo_]

Hey,

Thanks. Yes it is a bit ugly but the code is more complicated than above.. The classes have unique id's and so the == overload just checked that rather than everything in the class.

I found another way to do it that was a bit cleaner (well I hope) by iterating over a QMap of these id's.

Thanks though!

:-)

[d_stranz]

But do you realize what was wrong with your original code?

MyPointer *mp = new MyPointerBase(p2[i]);

MyPointer *mp = new MyPointerBase(p2[i]);

To copy to clipboard, switch view to plain text mode 

In this line you create a new instance of MyPointerBase using a pointer to another instance of MyPointer. I hope you understand that this is not a copy constructor.

if (p1.isEmpty() || !p1.contains(mp))

if (p1.isEmpty() || !p1.contains(mp))

To copy to clipboard, switch view to plain text mode 

In the very next line you ask the QList if it contains the pointer you just created. Of course it doesn't - you just made it, and have not inserted it into the container yet.

I think you have some very basic confusion between object instances and pointers to object instances. Your QList is a container of pointers and therefore any comparison is between the pointers themselves, not the object instances those pointers point to. So your class operator==() will never be called during any QList method that compares members of the list.

Even so, your attempt to overload operator==() is just off track.

// This really isn't a class member at all.
// This operator would be invoked through a call like
// if ( *mp1 == *mp2 )
bool operator== (MyPointerBase &pb1, MyPointerBase &pb2) {
	return (pb1.id == pb2.id);
}
 
// This one is a class member, but it's wrong.
// This one would get called through code like
// if ( *mp1 == mp2 )
// or
// if ( mp1->operator==( mp2 ) )
// which is downright weird.  How would your replacement ever understand code like that?
 
bool MyPointerBase::operator== (const MyPointerBase *other) const {
	return (this->id == other->id);
}

// This really isn't a class member at all.
// This operator would be invoked through a call like
// if ( *mp1 == *mp2 )
bool operator== (MyPointerBase &pb1, MyPointerBase &pb2) {
	return (pb1.id == pb2.id);
}

// This one is a class member, but it's wrong.
// This one would get called through code like
// if ( *mp1 == mp2 )
// or
// if ( mp1->operator==( mp2 ) )
// which is downright weird.  How would your replacement ever understand code like that?

bool MyPointerBase::operator== (const MyPointerBase *other) const {
	return (this->id == other->id);
}

To copy to clipboard, switch view to plain text mode 

If you want a class operator==(), then you write this:

bool MyPointerBase::operator==( const MyPointerBase & other ) const
{
   return id == other.id;
}

bool MyPointerBase::operator==( const MyPointerBase & other ) const
{
   return id == other.id;
}

To copy to clipboard, switch view to plain text mode 

and call it using pointers to class instances like this:

// In ALL cases mp1 and mp2 must be non-NULL
if ( *mp1 == *mp2 ) 
{
  // ...
}
// or
 
if ( mp1->operator==( *mp2 ) )
{
  // ...
}
 
// or
 
MyClass & mp1Ref = *mp1;
MyClass & mp2Ref = *mp2;
if ( mp1Ref == mp2Ref )
{
  // ...
}

// In ALL cases mp1 and mp2 must be non-NULL
if ( *mp1 == *mp2 ) 
{
  // ...
}
// or

if ( mp1->operator==( *mp2 ) )
{
  // ...
}

// or

MyClass & mp1Ref = *mp1;
MyClass & mp2Ref = *mp2;
if ( mp1Ref == mp2Ref )
{
  // ...
}

To copy to clipboard, switch view to plain text mode 

[jamo_]

Perfect. Thanks for the pointers.. Finally got it to work!

I think I understand it now - definitely now it isn't 1am!