can't use QVector::push_back on QUdpSocket

mzimmers

@eyllanesc oh, geez, I keep forgetting that...thanks.

Now, the QVector is actually a member variable; I need to use its contents outside of the routine this code is in. When the routine is called, it needs to clear the vector first. Do I understand that I need to call clear() and then squeeze() to avoid memory leaks? I'm using 5.14.2.

Thanks again.

Bonnie

@mzimmers
Before calling clear(), you should assure that the pointers have been freed.
If not, using qDeleteAll() before clear().

mzimmers

@Bonnie ah yes, that's precisely what I need. Thanks, Bonnie.

mzimmers

OK, I thought I had a handle on this, but the compiler is telling me differently.

Background: my app needs to maintain a list of "usable" network interfaces (definition of usable not important here). My thought was to create a struct:

struct SockInfo
{
    QNetworkInterface nic;
    QNetworkAddressEntry addr;
    QUdpSocket sockMulticast;
    QUdpSocket sockBroadcast;
};

and have a QList of these as a member variable. But...when I try to assign a value to an element, I get a compiler error (for the reasons given in prior posts).

By converting the socket members to pointers, I can do this (this code is executed after the list is created):

for (it = m_socks.begin(); it != m_socks.end(); ++it)
{
	pSockMulticast = new QUdpSocket;
	SockInfo *p;
	p = it;
	p->sockMulticast = pSockMulticast;

But this doesn't seem right -- I don't want a new socket; I want to alter the value of an entry in my list.

So...I guess I don't really understand this after all. Any explanations, or ideas on doing this better, are most welcome.

Thanks...

SGaist

Hi,

In what way do you want to alter them ?

mzimmers

@SGaist one example is that the nic and addr will be set at app startup, but the sockets won't be filled in until later. Plus, if I need to add a connected socket to the struct, and that sock gets disconnected, I need to alter the stored data to reflect that.

eyllanesc

@mzimmers Use

struct SockInfo
{
    QNetworkInterface nic;
    QNetworkAddressEntry addr;
    QUdpSocket *sockMulticast;
    QUdpSocket *sockBroadcast;
};

On the other hand I recommend you review your basic concepts of C++

mzimmers

@eyllanesc said in can't use QVector::push_back on QUdpSocket:

On the other hand I recommend you review your basic concepts of C++

Can you elaborate?

eyllanesc

@mzimmers For example when using the copy constructor, clearly when doing your_struct.sockMulticast = foo you are trying to use the copy constructor since you are trying to copy the foo object to the sockMulticast attribute of the structure. And as I already pointed out the QObjects are not copyable, the copyable ones are the pointers. Or what is the difference of T with T* and understand that it is a pointer, and that you cannot assign a T* to T, nor T to T*.

mzimmers

OK, let's do this one step at a time. This is partial code from my header file:

struct SockInfo
{
    QNetworkInterface nic;
    QNetworkAddressEntry addr;
    QUdpSocket *sockMulticast;
    QUdpSocket *sockBroadcast;
};
class UdpSocket : public QObject
{
	QVector<SockInfo> m_socks;
}

and here's where I create an entry:

void UdpSocket::getHostAddrs()
{
        SockInfo sockInfo;
	sockInfo.nic = *it_qni;
	sockInfo.addr = addrEntries[i];
	m_socks.append(sockInfo);
}

(Obviously some stuff is left out, but I think this demonstrates what I'm attempting.)

Does this look OK so far?

JonB

@mzimmers
Yes :)

Notice that m_socks.append(sockInfo) copies your local SockInfo sockInfo variable as a new element into m_socks. That's why your QUdpSockets needed to be pointers.

Though if you'd like us (me) to nitpick (probably not!): reading your code I'd be surprised when I discover a class named UdpSocket holds an array of (pairs of) QUdpSocket, so a whole bunch of QUdpSockets. Maybe at least UdpSockets, or UdpSocketCollection/SockInfoCollection or similar, something to indicate it holds a bunch of them? Perhaps it's just me....

mzimmers

@JonB thanks for the confirmation, and I agree entirely about the unfortunate name of this class; that will get remedied.

In looking over an earlier post of mine, I think I caused some confusion about what I was originally doing. In my struct, I had converted the QUdpSocket elements to pointers; I just didn't post the code. Mea culpa.

I'm still having some difficulty; unfortunately, my VPN is down, so I can't post code right now. I'll post code when I can, but the essence is, I couldn't make a QVector of SockInfo. I had to make a QVector of Sockinfo *. Apart from seeming like an unnecessary level of indirection, it also made the iterator code uglier than farm subsidies.

Anyway, like I said, I'll post more code when I can. Thanks...

JonB

@mzimmers
My thought would be that the members of SockInfo you show are copyable (pointers to all QObjects now), so I would have expected QVector<SockInfo> m_socks; to be OK.

mzimmers

@JonB actually, that's not quite how I did it. I made the QUdpSocket members pointers, but not the other members. Perhaps this was my undoing.

JonB

@mzimmers Show code when you're ready :)

mzimmers

@JonB said in can't use QVector::push_back on QUdpSocket:

@mzimmers
My thought would be that the members of SockInfo you show are copyable (pointers to all QObjects now), so I would have expected QVector<SockInfo> m_socks; to be OK.

You're right -- that is OK; I don't know what my problem was, but it was something else (I need to quit working late at night).

Here's the code -- preliminary testing indicates it's working, but I invite all critique/comments.

struct SockInfo
{
    QNetworkInterface nic;
    QNetworkAddressEntry addr;
    QUdpSocket *sockMulticast;
    QUdpSocket *sockBroadcast;
};
class UdpSocket : public QObject
{
private:
    QVector<SockInfo> m_socks;
}
// creating the list, and populating some of the members.
    QList<QNetworkAddressEntry> addrs; 
    QList<QNetworkAddressEntry>::iterator addr;
	addrs = nic->addressEntries();

	// for each address entry
	for (addr = addrs.begin(); addr != addrs.end(); ++addr)
	{
		qha = addr->ip();
		if (qha.protocol() == QAbstractSocket::IPv4Protocol) // currently ignoring IP6 addresses.
		{
			sockInfo = new SockInfo;
			sockInfo->nic = *nic;
			sockInfo->addr = *addr;
			m_socks.append(*sockInfo);
			break;
		}
	}
// updating the list.
    QVector<SockInfo>::iterator sock;
    for (sock = m_socks.begin(); sock != m_socks.end(); ++sock)
    {
        pSockMulticast = new QUdpSocket;
        pSockBroadcast = new QUdpSocket;
        sock->sockMulticast = pSockMulticast;
        sock->sockBroadcast = pSockBroadcast;
...

Thanks...

JonB

@mzimmers
Briefly:

        sockInfo = new SockInfo;
        m_socks.append(*sockInfo);

Unless the sockinfo variable used here is a member variable of the class and you delete it in class destructor, or maybe you delete it in omitted code, this leaks. The sockinfo = new SockInfo instance is not itself being put in m_socks with m_socks.append(*sockInfo);, that copies it. Why do you new your SockInfos at all? If you are not intending this, you could have had:

QSockInfo sockinfo;
m_socks.append(sockInfo);

It's OK to use a local/stack variable which goes out of scope because m_socks.append(sockInfo) is storing a copy of it.

Separately

        sock->sockMulticast = pSockMulticast;
        sock->sockBroadcast = pSockBroadcast;

If you are updating, what are you doing about the sock->sockMulticast/Broadcast which is currently there, which itself was previously newed here & elsewhere? Maybe that's in your omitted code.

mzimmers

I can see that I need to rename several variables...it seems like I actually tried to make this confusing.

I left out the code that ties all this together. Whenever the main routine is invoked, it first calls this:

void UdpSocket::cleanup()
{
    QVector<SockInfo>::iterator it;

    for (it = m_socks.begin(); it != m_socks.end(); ++it)
    {
        if (it->sockMulticast->isValid())
        {
            delete it->sockMulticast;
        }
        if (it->sockBroadcast->isValid())
        {
            delete it->sockBroadcast;
        }
    }
    qDeleteAll(m_socks.begin(), m_socks.end());
    m_socks.clear();

...and then calls the code that creates the list, and then calls the code that creates the QUdpSockets that go into that list. So, I think I'm good on memory leaks (but of course I could be wrong).

Your point about using a struct instead of a pointer to a struct is well-taken; I've changed that.

Thanks...

EDIT: since no one else commented on my implementation, I'm going to assume that it's tolerable. Marking thread as solved. Thanks to all who helped.