Problem when returning a QVector from a function

[meazza]

God day to you my fellow programmers.

I have a little problem when using two different QVector objects. One is declared as

QVector<QDSWireObject*> m_wireVector;

QVector<QDSWireObject*> m_wireVector;

To copy to clipboard, switch view to plain text mode 

.
The other one is declared as

QVector< QVector<QDSWireObject*> > m_vectorVector;

QVector< QVector<QDSWireObject*> > m_vectorVector;

To copy to clipboard, switch view to plain text mode 

.

And now i have two different functions which is

void QDSMainWindow::deleteWire(QDSWireObject *wire)
{
     QVector<QDSWireObject*> objectVector = findObjectVector(wire);
 
     if(!objectVector.isEmpty())
     {
         Q_FOREACH(QDSWireObject* item, objectVector)
         {
             if(item == wire)
             {
                 int index = objectVector.indexOf(wire);
 
                 if(item->getConnectedBefore())
                    objectVector.at(index - 1)->setConnectedAfter(0);
 
                 if(item->getConnectedAfter())
                    objectVector.at(index +1 )->setConnectedBefore(0);
 
                 item->deleteLater();
                 objectVector.remove(objectVector.indexOf(item));
                 qDebug() << objectVector.size();
             }
         }
     }
}

void QDSMainWindow::deleteWire(QDSWireObject *wire)
{
     QVector<QDSWireObject*> objectVector = findObjectVector(wire);

     if(!objectVector.isEmpty())
     {
         Q_FOREACH(QDSWireObject* item, objectVector)
         {
             if(item == wire)
             {
                 int index = objectVector.indexOf(wire);

                 if(item->getConnectedBefore())
                    objectVector.at(index - 1)->setConnectedAfter(0);

                 if(item->getConnectedAfter())
                    objectVector.at(index +1 )->setConnectedBefore(0);

                 item->deleteLater();
                 objectVector.remove(objectVector.indexOf(item));
                 qDebug() << objectVector.size();
             }
         }
     }
}

To copy to clipboard, switch view to plain text mode 

This function makes a call to this other function that I am using

const QVector<QDSWireObject*>& QDSMainWindow::findObjectVector(QDSWireObject *wire)
{
    Q_FOREACH(QVector<QDSWireObject*> item, m_vectorVector)
    {
        if(item.contains(wire))
        {
            int index = m_vectorVector.indexOf(item);
            return m_vectorVector.at(index);
        }
    }
 
    return QVector<QDSWireObject*>();
}

const QVector<QDSWireObject*>& QDSMainWindow::findObjectVector(QDSWireObject *wire)
{
    Q_FOREACH(QVector<QDSWireObject*> item, m_vectorVector)
    {
        if(item.contains(wire))
        {
            int index = m_vectorVector.indexOf(item);
            return m_vectorVector.at(index);
        }
    }

    return QVector<QDSWireObject*>();
}

To copy to clipboard, switch view to plain text mode 

And now problem here is that the QVector that the findObjectVector() function returns seems to be a copy of the one from the stored in m_vectorVector. Because when i remove an element it gets removed from the vector and the size changes but if I make several calls to the function I can tell that the vector size of the vector stored in m_vectorVector is not changing. Which I don't really understand why because I am returning a reference to the object.

Hopefully someone can help me out here and see what my problem is.

[wysota]

Look at line 3 of your code, you are making a copy of the vector there. You should be getting a warning from the compiler about returning a reference to a local object as well. By the way, your find method is not very optimal.

[meazza]

Not getting any warnings regarding this. I tried to change the function without the the code on line 3.

void QDSMainWindow::deleteWire(QDSWireObject *wire)
{
     if(!findObjectVector(wire).isEmpty())
     {
         Q_FOREACH(QDSWireObject* item, findObjectVector(wire))
         {
             if(item == wire)
             {
                 int index = findObjectVector(wire).indexOf(wire);
 
                 if(item->getConnectedBefore())
                    findObjectVector(wire).at(index - 1)->setConnectedAfter(0);
 
                 if(item->getConnectedAfter())
                    findObjectVector(wire).at(index +1 )->setConnectedBefore(0);
 
                 item->deleteLater();
                 findObjectVector(wire).remove(findObjectVector(wire).indexOf(item));
                 qDebug() << findObjectVector(wire).size();
             }
         }
     }
}

void QDSMainWindow::deleteWire(QDSWireObject *wire)
{
     if(!findObjectVector(wire).isEmpty())
     {
         Q_FOREACH(QDSWireObject* item, findObjectVector(wire))
         {
             if(item == wire)
             {
                 int index = findObjectVector(wire).indexOf(wire);

                 if(item->getConnectedBefore())
                    findObjectVector(wire).at(index - 1)->setConnectedAfter(0);

                 if(item->getConnectedAfter())
                    findObjectVector(wire).at(index +1 )->setConnectedBefore(0);

                 item->deleteLater();
                 findObjectVector(wire).remove(findObjectVector(wire).indexOf(item));
                 qDebug() << findObjectVector(wire).size();
             }
         }
     }
}

To copy to clipboard, switch view to plain text mode 

and I get the error C:\Documents and Settings\KY\Mina dokument\Dropbox\Qt Creator\DigitalSim\qmake\..\src\src\QDSMainWindow. cpp:204: error: passing 'const QVector<QDSWireObject*>' as 'this' argument of 'void QVector<T>::remove(int) [with T = QDSWireObject*]' discards qualifiers.

Why is this?

[wysota]

Because you are passing a const object to a non-const method. By the way... think whether calling findObjectVector() so many times in your deleteWire() method is really a good idea. I know CPU power is cheap nowadays, but is it really THAT cheap to execute the same O(n^2) method with the same argument a couple of times?

[meazza]

I know it is unnecessary to call it that many times but did this as a test to see if it would work instead of copying it.

---

Added after 5 minutes:

Look at line 3 of your code, you are making a copy of the vector there. You should be getting a warning from the compiler about returning a reference to a local object as well. By the way, your find method is not very optimal.

How could I optimize my find method?

[wysota]

Your whole approach is incorrect. Instead of trying to fix what you already have, throw it away and rewrite from scratch properly. Return indexes or pointers instead of references.

Here is what your findObjectVector() can look like:

int QDSMainWinwod::findObjectVector(QDSWireObject *wire) {
  for(int i=0;i<m_vectorVector.size();++i) {
    int index = m_vectorVector[i].indexOf(wire);
    if(index>=0) return i;
  }
  return -1;
}

int QDSMainWinwod::findObjectVector(QDSWireObject *wire) {
  for(int i=0;i<m_vectorVector.size();++i) {
    int index = m_vectorVector[i].indexOf(wire);
    if(index>=0) return i;
  }
  return -1;
}

To copy to clipboard, switch view to plain text mode 

It's still suboptimal because eventually you'll have to search through the vector again to find the index of the item, but it's a start... You can improve it like so:

int QDSMainWinwod::findObjectVector(QDSWireObject *wire, int *indexInVector = 0) {
  for(int i=0;i<m_vectorVector.size();++i) {
    int index = m_vectorVector[i].indexOf(wire);
    if(index>=0) {
        if(indexInVector) *indexInVector = index;
        return i;
    }
  }
  return -1;
}

int QDSMainWinwod::findObjectVector(QDSWireObject *wire, int *indexInVector = 0) {
  for(int i=0;i<m_vectorVector.size();++i) {
    int index = m_vectorVector[i].indexOf(wire);
    if(index>=0) {
        if(indexInVector) *indexInVector = index;
        return i;
    }
  }
  return -1;
}

To copy to clipboard, switch view to plain text mode 

and then:

void QDSMainWindow::deleteWire(QDSWireObject *wire) {
  int idx = -1;
  int vec = findObjectVector(wire, &idx);
  if(vec==-1) return;
  m_vectorVector[vec].removeAt(idx);
  delete wire;
}

void QDSMainWindow::deleteWire(QDSWireObject *wire) {
  int idx = -1;
  int vec = findObjectVector(wire, &idx);
  if(vec==-1) return;
  m_vectorVector[vec].removeAt(idx);
  delete wire;
}

To copy to clipboard, switch view to plain text mode