What's wrong with this class

[John Doe]

Hi,

I wrote a small class to enumerate available networks on a smartphone :

class CNetwork
{
public:
CNetwork() {};
CNetwork(CString& netName, GUID netguid):
_netname(netName), _netguid(netguid) {}

~CNetwork() {}

CString& getName() { return _netname; }
GUID getGuid() { return _netguid; }

private:
CString _netname;
GUID _netguid;
};
class CNetworkList
{
public:
typedef std::list<CNetwork*>::iterator NetworkListIt;

CNetworkList() {}

~CNetworkList()
{
Clear();
}

CNetworkList::CNetworkList(const CNetworkList& rhs) {
CopyObj(rhs);
}

CNetworkList& CNetworkList::operator=(const CNetworkList& rhs)
{
CopyObj(rhs);
return *this;
}

void CopyObj(const CNetworkList& rhs)
{
_netlist = rhs._netlist;
}

void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
}

void Add(CNetwork* network)
{
_netlist.push_back(network);
}

const CNetwork* getNetwork(CString netNameOrGuid)
{
if ((netNameOrGuid.GetAt(0) == '{') &&
netNameOrGuid.GetLength() == 39)
{
CLSID guid;
if
(SUCCEEDED(CLSIDFromString(netNameOrGuid.GetBuffer (),&guid)))
return getNetwork(guid);
}
else
{
NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if (!(*it)->getName().CompareNoCase(netNameOrGuid))
return (*it);
}
return NULL;
}

const CNetwork* getNetwork(CLSID guid)
{
if (!_netlist.empty())
Clear();

NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if ((*it)->getGuid() == guid)
return (*it);

return NULL;
}

private:
std::list<CNetwork*_netlist;
};

CNetworkList getNetworkList()
{
int i = 0;
HRESULT hResult;
CNetworkList netList;

while( ConnMgrEnumDestinations( i, &connInfo ) == 0 )
{
CNetwork* pNetWork = new
CNetwork(CString(connInfo.szDescription), connInfo.guid);
if (pNetWork)
{
netList.Add(pNetWork);
}

i++;
}
}

When I call this code :
m_NetworkList = getNetworkList();
I got an assert in a Cstring desctructor so I suppose my class is doing
wrong...
When I debug in step by step I don't really understand the calls, it
seems Clear() is called why it shoudn't.

[Victor Bazarov]

John Doe wrote:

Hi,

I wrote a small class to enumerate available networks on a smartphone :

class CNetwork

Why do you need the 'C' in front of the name? I can understand 'SP'
(for smartphone), but 'C'?

{
public:
CNetwork() {};
CNetwork(CString& netName, GUID netguid):
_netname(netName), _netguid(netguid) {}

If your class owns the member '_netname' (which it probably should, as
you designed it), consider passing the initialiser for it as a reference
to const:

CNetwork(CString const& netName, GUID...

>
~CNetwork() {}

CString& getName() { return _netname; }

This is a bad idea. You are exposing the innards of your class to any
change that you can't control or even know about. If your 'Network'
needs to report its name, this member has to be 'const' and should
return a reference to const:

CString const& getName() cosnt { return _netname; }

GUID getGuid() { return _netguid; }

Same here:

GUID getGuid() const { return _netguid; }

>
private:
CString _netname;
GUID _netguid;
};
class CNetworkList
{
public:
typedef std::list<CNetwork*>::iterator NetworkListIt;

Does this need to be public?

>
CNetworkList() {}

~CNetworkList()
{
Clear();
}

CNetworkList::CNetworkList(const CNetworkList& rhs) {
CopyObj(rhs);
}

What is that? Why couldn't you just use the normal copy constructor form:

CNetworkList::CNetworkList(const CNetworkList& rhs) :
netlist(rhs.netlist) {}

? And if it matters, this is pretty much unnecessary because the
compiler will provide you with the copy constructor that does exactly that.

>
CNetworkList& CNetworkList::operator=(const CNetworkList& rhs)
{
CopyObj(rhs);
return *this;
}

Again, the assignment operator provided by the compiler will work just
fine, most likely. You don't have to provide your own.

>
void CopyObj(const CNetworkList& rhs)
{
_netlist = rhs._netlist;
}

void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
}

void Add(CNetwork* network)
{
_netlist.push_back(network);
}

Now, you do realise that your list is made the owner of that pointer
here, right?

>
const CNetwork* getNetwork(CString netNameOrGuid)

The interface of this function is better if (a) it's 'const' and (b) its
argument is not passed by value:

const CNetwork* getNetwork(CStirng const& netNameOrGuid) const

{
if ((netNameOrGuid.GetAt(0) == '{') &&
netNameOrGuid.GetLength() == 39)
{
CLSID guid;
if
(SUCCEEDED(CLSIDFromString(netNameOrGuid.GetBuffer (),&guid)))
return getNetwork(guid);
}
else
{
NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if (!(*it)->getName().CompareNoCase(netNameOrGuid))
return (*it);
}
return NULL;
}

const CNetwork* getNetwork(CLSID guid)

Same comment as above,

const CNetwork* getNetwork(CLSID guid) const

{
if (!_netlist.empty())
Clear();

NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if ((*it)->getGuid() == guid)
return (*it);

return NULL;
}

private:
std::list<CNetwork*_netlist;
};

CNetworkList getNetworkList()
{
int i = 0;
HRESULT hResult;
CNetworkList netList;

while( ConnMgrEnumDestinations( i, &connInfo ) == 0 )
{
CNetwork* pNetWork = new
CNetwork(CString(connInfo.szDescription), connInfo.guid);
if (pNetWork)

'new' never returns NULL. You should, however, surround it with
'try-catch' since 'new' throws 'std::bad_alloc' if something happens.

{
netList.Add(pNetWork);
}

i++;
}
}

When I call this code :
m_NetworkList = getNetworkList();

Where?

I got an assert in a Cstring desctructor so I suppose my class is doing
wrong...
When I debug in step by step I don't really understand the calls, it
seems Clear() is called why it shoudn't.

Post complete code and provide a test driver that would produce the
network (instead of 'ConnMgrEnumDesitnations' which C++ doesn't have).

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[John Doe]

Victor Bazarov wrote:

John Doe wrote:

So here is the update code (without the try for new I will do it later)
but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...
//============================================//
// NetworkManager.h
//============================================//
struct DeletePointer {
template<typename T>
void operator()(const T* ptr) const
{
delete ptr;
}
};

class Network
{
public:
Network() {};
Network(CString const& netName, GUID netguid):
_netname(netName), _netguid(netguid) {}

~Network() {}

CString const& getName() { return _netname; }
GUID getGuid() const { return _netguid; }

private:
CString _netname;
GUID _netguid;
};
class NetworkList
{
typedef std::list<Network*>::iterator NetworkListIt;

public:

NetworkList()
{
}

~NetworkList()
{
Clear();
}

void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
}

void Add(Network* network)
{
if (network)
_netlist.push_back(network);
}
const Network* getNetwork(CString const& netNameOrGuid) const
{
if ((netNameOrGuid.GetAt(0) == '{') &&
netNameOrGuid.GetLength() == 39)
{
CLSID guid;
if (SUCCEEDED(CLSIDFromString((LPOLESTR)(LPCTSTR)netN ameOrGuid,&guid)))
{
return getNetwork(guid);
}
}
else
{
std::list<Network*>::const_iterator it;
//NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if (!(*it)->getName().CompareNoCase(netNameOrGuid))
return (*it);
}
return NULL;
}

const Network* getNetwork(CLSID guid) const
{
if (!_netlist.empty())
Clear();

std::list<Network*>::const_iterator it;
//NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if ((*it)->getGuid() == guid)
return (*it);

return NULL;
}

private:
std::list<Network*_netlist;
};

//============================================//
// NetworkManager.cpp
//============================================//
NetworkList getNetworkList()
{
NetworkList netList;

// Simulate we retrieve network list from OS
GUID guid;
netList.Add(new Network(_T("Network1"), guid));
netList.Add(new Network(_T("Network2"), guid));

return netList;
}

//============================================//
// Testcase
//============================================//

void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}

When the netList is destroyed I get a debug assertion due to CString
object :

void Release() throw()
{
ATLASSERT( nRefs != 0 ); <<<<< !!!!!!!!!

if( _AtlInterlockedDecrement( &nRefs ) <= 0 )
{
pStringMgr->Free( this );
}
}

[Obnoxious User]

On Fri, 24 Oct 2008 17:18:15 +0200, John Doe wrote:

Victor Bazarov wrote:

>John Doe wrote:

So here is the update code (without the try for new I will do it later)
but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...

[snip]

>
void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer());
}

How many times is this function called in your test case?
Most probably two times, right?
And what does it do each time?

[snip]

>
void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}

You return a *copy* of the NetworkList you build in the function.
The first copy dies and deallocates via Clear() all the pointers so
the copy contains invalid pointers, which are deallocated yet again,
that's where the CString fails it assertions, becuase it is already
destroyed.

>
When the netList is destroyed I get a debug assertion due to CString
object :

void Release() throw()
{
ATLASSERT( nRefs != 0 ); <<<<< !!!!!!!!!

if( _AtlInterlockedDecrement( &nRefs ) <= 0 ) {
pStringMgr->Free( this );
}
}

--
OU
Remember 18th of June 2008, Democracy died that afternoon.
http://frapedia.se/wiki/Information_in_English

[John Doe]

Obnoxious User wrote:

On Fri, 24 Oct 2008 17:18:15 +0200, John Doe wrote:

>Victor Bazarov wrote:

>>John Doe wrote:

So here is the update code (without the try for new I will do it later)
but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...

[snip]

> void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer());
}

How many times is this function called in your test case?
Most probably two times, right?
And what does it do each time?

[snip]

>void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}

You return a *copy* of the NetworkList you build in the function.
The first copy dies and deallocates via Clear() all the pointers so
the copy contains invalid pointers, which are deallocated yet again,
that's where the CString fails it assertions, becuase it is already
destroyed.

yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?

void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}
Maybe it's a bad idea and I should do something like :

NetworkList netList;
getNetworkList(netList);

>When the netList is destroyed I get a debug assertion due to CString
object :

void Release() throw()
{
ATLASSERT( nRefs != 0 ); <<<<< !!!!!!!!!

if( _AtlInterlockedDecrement( &nRefs ) <= 0 ) {
pStringMgr->Free( this );
}
}

[peter koch]

On 24 Okt., 18:14, John Doe <[e-mail]:

Obnoxious User wrote:

On Fri, 24 Oct 2008 17:18:15 +0200, John Doe wrote:

Victor Bazarov wrote:
John Doe wrote:
So here is the update code (without the try for new I will do it later)
but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...

[snip]

* * * *void Clear()
* * * *{
* * * * *for_each( _netlist.begin(), _netlist.end(), DeletePointer());
* * * *}

How many times is this function called in your test case?
Most probably two times, right?
And what does it do each time?

[snip]

void OnGettingNetworkList()
{
* * NetworkList netList = getNetworkList();
}

You return a *copy* of the NetworkList you build in the function.
The first copy dies and deallocates via Clear() all the pointers so
the copy contains invalid pointers, which are deallocated yet again,
that's where the CString fails it assertions, becuase it is already
destroyed.

yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?

void OnGettingNetworkList()
{
* * * NetworkList netList = getNetworkList();

}

Maybe it's a bad idea and I should do something like :

NetworkList netList;
getNetworkList(netList);

I don't see what that would help. What I don't understand is why the
networklist is a list of pointers (typedef std::list<Network*>). It
looks more natural to me to have it as a list of values
(std::list<Network>). This also simplifies your design quite a lot.

/Peter

[Obnoxious User]

On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:

Obnoxious User wrote:

>On Fri, 24 Oct 2008 17:18:15 +0200, John Doe wrote:

>>Victor Bazarov wrote:
John Doe wrote:
So here is the update code (without the try for new I will do it
later) but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...

[snip]

>> void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer());
}

How many times is this function called in your test case? Most probably
two times, right?
And what does it do each time?

[snip]

>>void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}

You return a *copy* of the NetworkList you build in the function. The
first copy dies and deallocates via Clear() all the pointers so the
copy contains invalid pointers, which are deallocated yet again, that's
where the CString fails it assertions, becuase it is already destroyed.

yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?

You're missing a proper copy constructor.

--
OU
Remember 18th of June 2008, Democracy died that afternoon.
http://frapedia.se/wiki/Information_in_English

[Mosfet]

Obnoxious User a écrit :

On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:

>Obnoxious User wrote:

>>On Fri, 24 Oct 2008 17:18:15 +0200, John Doe wrote:

Victor Bazarov wrote:
John Doe wrote:
So here is the update code (without the try for new I will do it
later) but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...
[snip]
void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer());
}
How many times is this function called in your test case? Most probably
two times, right?
And what does it do each time?

[snip]
void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}
You return a *copy* of the NetworkList you build in the function. The
first copy dies and deallocates via Clear() all the pointers so the
copy contains invalid pointers, which are deallocated yet again, that's
where the CString fails it assertions, becuase it is already destroyed.

yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?

You're missing a proper copy constructor.

Finally I am using getNetworkList like this :

void getNetworkList(NetworkList& netList)
{
....
}
getNetworkList(m_NetworkList);
and now it works fine.
However now that I have added const everywhere I cannot even do this :

CString strTmp = pNetwork->getName();
or LPCTSTR szName = pNetwork->getName();


That's why I think const brings more issues than it solves ...

[Mosfet]

Mosfet a écrit :

Obnoxious User a écrit :

>On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:

>>Obnoxious User wrote:
On Fri, 24 Oct 2008 17:18:15 +0200, John Doe wrote:

Victor Bazarov wrote:
>John Doe wrote:
So here is the update code (without the try for new I will do it
later) but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'
>
so I have added const to Clear()...
>
>
[snip]
void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer());
}
>
>
How many times is this function called in your test case? Most probably
two times, right?
And what does it do each time?

[snip]
void OnGettingNetworkList()
{
NetworkList netList = getNetworkList();
}
You return a *copy* of the NetworkList you build in the function. The
first copy dies and deallocates via Clear() all the pointers so the
copy contains invalid pointers, which are deallocated yet again, that's
where the CString fails it assertions, becuase it is already destroyed.

yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?

You're missing a proper copy constructor.

Finally I am using getNetworkList like this :

void getNetworkList(NetworkList& netList)
{
...
}
getNetworkList(m_NetworkList);
and now it works fine.
However now that I have added const everywhere I cannot even do this :

CString strTmp = pNetwork->getName();
or LPCTSTR szName = pNetwork->getName();


That's why I think const brings more issues than it solves ...

YHum sorry with the full code it's better :

const Network* pNetwork = m_NetworkList.getNetwork(lpAttrVal);
const CString& strTmp = pNetwork->getName();

I get :

error C2662: 'Network::getName' : cannot convert 'this' pointer from
'const Network' to 'Network &'

[peter koch]

On 24 Okt., 20:05, Mosfet <[e-mail]:

Obnoxious User a écrit :

On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:

Obnoxious User wrote:

[snip]

>

>How many times is this function called in your test case? Most probably
two times, right?
And what does it do each time?

>[snip]
void OnGettingNetworkList()
{
* * NetworkList netList = getNetworkList();
}
You return a *copy* of the NetworkList you build in the function. The
first copy dies and deallocates via Clear() all the pointers so the
copy contains invalid pointers, which are deallocated yet again, that's
where the CString fails it assertions, becuase it is already destroyed.

yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?

You're missing a proper copy constructor.

Finally I am using getNetworkList like this :

void getNetworkList(NetworkList& netList)
{
...

}

getNetworkList(m_NetworkList);
and now it works fine.

No it isn't. Not unless you do a fair deal of work in the function (a
deep copy), and in that case, you might just as well have followed my
advice in the first place.

>
However now that I have added const everywhere I cannot even do this :

CString strTmp = pNetwork->getName();
or LPCTSTR szName = pNetwork->getName();

That's why I think const brings more issues than it solves ...

That is because you have put your consts in the wrong places or
because CString is broken with respect to constness - I don't really
know that class. Why dount you simply use std::wstring?

/Peter

[Mosfet]

peter koch a écrit :

On 24 Okt., 20:05, Mosfet <[e-mail]:

>Obnoxious User a écrit :

>>On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:
Obnoxious User wrote:

[snip]

>>>>How many times is this function called in your test case? Most probably
two times, right?
And what does it do each time?
[snip]
>void OnGettingNetworkList()
>{
> NetworkList netList = getNetworkList();
>}
You return a *copy* of the NetworkList you build in the function. The
first copy dies and deallocates via Clear() all the pointers so the
copy contains invalid pointers, which are deallocated yet again, that's
where the CString fails it assertions, becuase it is already destroyed.
yes thanks!!!! I knew it was something like that but I couldn't find it.
How can I solve this ? If I remove my Clear() method how can I be sure
that when netList is destroyed, there is no memory leak ?
You're missing a proper copy constructor.

Finally I am using getNetworkList like this :

void getNetworkList(NetworkList& netList)
{
...

}

getNetworkList(m_NetworkList);
and now it works fine.

No it isn't. Not unless you do a fair deal of work in the function (a
deep copy), and in that case, you might just as well have followed my
advice in the first place.

If I knew what you mean I won't be posting here
Maybe it doesn't work but my application seems ok!
I forgot to say now getNetworkList is now declared to take a reference
on a NetworkList so there is no copy anymore.
:
void getNetworkList(NetworkList& netList)
{
GUID guid;
netList.Add(new Network(_T("Network1"), guid));
netList.Add(new Network(_T("Network2"), guid));
}

>However now that I have added const everywhere I cannot even do this :

>CString strTmp = pNetwork->getName();
or LPCTSTR szName = pNetwork->getName();

That's why I think const brings more issues than it solves ...

That is because you have put your consts in the wrong places or
because CString is broken with respect to constness - I don't really
know that class. Why dount you simply use std::wstring?

/Peter

Same issue with std::wstring..

const Network* pNetwork = m_NetworkList.getNetwork(lpAttrVal);
const std::wstring& strTmp = pNetwork->getName();

error C2662: 'Network::getName' : cannot convert 'this' pointer from
'const Network' to 'Network &'

[Obnoxious User]

On Fri, 24 Oct 2008 20:32:01 +0200, Mosfet wrote:

peter koch a écrit :

>On 24 Okt., 20:05, Mosfet <[e-mail]:

>>Obnoxious User a écrit :

On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:
Obnoxious User wrote:

[snip]

>>>>>How many times is this function called in your test case? Most
>probably two times, right?
>And what does it do each time?
>[snip]
>>void OnGettingNetworkList()
>>{
>> NetworkList netList = getNetworkList();
>>}
>You return a *copy* of the NetworkList you build in the function.
>The first copy dies and deallocates via Clear() all the pointers so
>the copy contains invalid pointers, which are deallocated yet
>again, that's where the CString fails it assertions, becuase it is
>already destroyed.
yes thanks!!!! I knew it was something like that but I couldn't find
it. How can I solve this ? If I remove my Clear() method how can I
be sure that when netList is destroyed, there is no memory leak ?
You're missing a proper copy constructor.
Finally I am using getNetworkList like this :

void getNetworkList(NetworkList& netList) {
...

}

getNetworkList(m_NetworkList);
and now it works fine.

No it isn't. Not unless you do a fair deal of work in the function (a
deep copy), and in that case, you might just as well have followed my
advice in the first place.

If I knew what you mean I won't be posting here Maybe it doesn't work
but my application seems ok! I forgot to say now getNetworkList is now
declared to take a reference on a NetworkList so there is no copy
anymore. :
void getNetworkList(NetworkList& netList) {
GUID guid;
netList.Add(new Network(_T("Network1"), guid)); netList.Add(new
Network(_T("Network2"), guid));
}

>>However now that I have added const everywhere I cannot even do this :
CString strTmp = pNetwork->getName(); or LPCTSTR szName =
pNetwork->getName();

That's why I think const brings more issues than it solves ...

Const-correctness is an important concept to understand in C++.

>>
That is because you have put your consts in the wrong places or because
CString is broken with respect to constness - I don't really know that
class. Why dount you simply use std::wstring?

Same issue with std::wstring..

const Network* pNetwork = m_NetworkList.getNetwork(lpAttrVal); const
std::wstring& strTmp = pNetwork->getName();

error C2662: 'Network::getName' : cannot convert 'this' pointer from
'const Network' to 'Network &'

Post a minimal *compilable* (no Windows dependencies) program
stripped of everything not directly essential to demonstrate the
problem.

--
OU
Remember 18th of June 2008, Democracy died that afternoon.
http://frapedia.se/wiki/Information_in_English

[Mosfet]

Obnoxious User a écrit :

On Fri, 24 Oct 2008 20:32:01 +0200, Mosfet wrote:

>peter koch a écrit :

>>On 24 Okt., 20:05, Mosfet <[e-mail]:
Obnoxious User a écrit :

On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:
>Obnoxious User wrote:
[snip]
>>How many times is this function called in your test case? Most
>>probably two times, right?
>>And what does it do each time?
>>[snip]
>>>void OnGettingNetworkList()
>>>{
>>> NetworkList netList = getNetworkList();
>>>}
>>You return a *copy* of the NetworkList you build in the function.
>>The first copy dies and deallocates via Clear() all the pointers so
>>the copy contains invalid pointers, which are deallocated yet
>>again, that's where the CString fails it assertions, becuase it is
>>already destroyed.
>yes thanks!!!! I knew it was something like that but I couldn't find
>it. How can I solve this ? If I remove my Clear() method how can I
>be sure that when netList is destroyed, there is no memory leak ?
You're missing a proper copy constructor.
Finally I am using getNetworkList like this :

void getNetworkList(NetworkList& netList) {
...

}

getNetworkList(m_NetworkList);
and now it works fine.
No it isn't. Not unless you do a fair deal of work in the function (a
deep copy), and in that case, you might just as well have followed my
advice in the first place.

If I knew what you mean I won't be posting here Maybe it doesn't work
but my application seems ok! I forgot to say now getNetworkList is now
declared to take a reference on a NetworkList so there is no copy
anymore. :
void getNetworkList(NetworkList& netList) {
GUID guid;
netList.Add(new Network(_T("Network1"), guid)); netList.Add(new
Network(_T("Network2"), guid));
}

>>>However now that I have added const everywhere I cannot even do this :
CString strTmp = pNetwork->getName(); or LPCTSTR szName =
pNetwork->getName();

That's why I think const brings more issues than it solves ...

Const-correctness is an important concept to understand in C++.

>>That is because you have put your consts in the wrong places or because
CString is broken with respect to constness - I don't really know that
class. Why dount you simply use std::wstring?

Same issue with std::wstring..

const Network* pNetwork = m_NetworkList.getNetwork(lpAttrVal); const
std::wstring& strTmp = pNetwork->getName();

error C2662: 'Network::getName' : cannot convert 'this' pointer from
'const Network' to 'Network &'

Post a minimal *compilable* (no Windows dependencies) program
stripped of everything not directly essential to demonstrate the
problem.

http://www.smartmobili.com/Downloads/NetWorkManager.zip

[James Kanze]

On Oct 24, 4:43*pm, Victor Bazarov <[e-mail]:

John Doe wrote:

I wrote a small class to enumerate available networks on a
smartphone :

class CNetwork

Why do you need the 'C' in front of the name? *I can
understand 'SP' (for smartphone), but 'C'?

I thought C was the prefix Microsoft reserved for MFC. With
namespaces, of course, you don't need such things at all
(although maybe the class should be in a SmartPhone namespace).

{
public:
* * CNetwork() {};
* * CNetwork(CString& netName, GUID netguid):
* * * _netname(netName), _netguid(netguid) {}

If your class owns the member '_netname' (which it probably
should, as you designed it), consider passing the initialiser
for it as a reference to const:

* * CNetwork(CString const& netName, GUID...

Well, I suppose it really depends on what CString is. But I
think this one is from MFC, and that it has basic copy
semantics, much like std::string (which he should probably
consider using, instead of CString), then the most natural form
is call by value. Of course, if he doesn't use call by value or
a const reference, he can't construct from anything but an
lvalue---no CString returned from a function, for example.

* * * ~CNetwork() {}

* * CString& getName() { return _netname; }

This is a bad idea. *You are exposing the innards of your
class to any change that you can't control or even know about.
*If your 'Network' needs to report its name, this member has
to be 'const' and should return a reference to const:

* * * CString const& getName() cosnt { return _netname; }

Again, returning a value is more natural. In this case,
returning even a const reference is dangerous, because it only
really works if the client copies it immediately (in which case,
RVO would have eliminated the copy in return by value);
otherwise, you have a potential problem with the lifetime of the
reference.

A string is a value; it's almost always wrong to have a
reference or a pointer to one.

Of course, making the function const is a good idea.

* * GUID getGuid() { return _netguid; }

Same here:

* * * GUID getGuid() const { return _netguid; }

private:
* * CString _netname;
* * GUID _netguid;
};

class CNetworkList
{
public:
* * typedef std::list<CNetwork*>::iterator NetworkListIt;

Does this need to be public?

* * CNetworkList() {}

* * ~CNetworkList()
* * {
* * * * Clear();
* * }

* * CNetworkList::CNetworkList(const CNetworkList& rhs) {
* * * * CopyObj(rhs);
* * }

What is that? *Why couldn't you just use the normal copy
constructor form:

* * * *CNetworkList::CNetworkList(const CNetworkList& rhs) :
* * * * * *netlist(rhs.netlist) {}

?

Because the semantics aren't correct. His list is a list of
pointers (which is probably the real error), and he needs to
copy the pointed to objects, or strange things are goind to
happen.

*And if it matters, this is pretty much unnecessary because
the compiler will provide you with the copy constructor that
does exactly that.

* * CNetworkList& CNetworkList::operator=(const CNetworkList& rhs)
* * {
* * * * CopyObj(rhs);
* * * * return *this;
* * }

Again, the assignment operator provided by the compiler will work just
fine, most likely. *You don't have to provide your own.

* * void CopyObj(const CNetworkList& rhs)
* * {
* * * * _netlist = rhs._netlist;
* * }

Except, of course, he got it wrong here. Given the semantics of
his destructor, if he's using pointers, he needs something like:

_netlist.clear() ;
for ( NetworkListIt iter = rhs._netlist.begin() ;
iter != rhs._netlist.end() ;
++ iter ) {
_netlist.push_back( new CNetwork( **iter ) ) ;
}

Another good reason not to use pointers.

* * void Clear()
* * {
* * * * for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
* * }

With values, rather than pointers, this becomes simply:
_netlist.clear() ;

* * void Add(CNetwork* network)
* * {
* * * * _netlist.push_back(network);
* * }

Now, you do realise that your list is made the owner of that
pointer here, right?

That's the real problem. It is as far as the destructor's
concerned. But not according to the copy constructor nor the
assignment operator. It's not surprising that he's having
problems.

Again, CNetwork is a value, not an entity (at least from what
little I can see here). That means: no pointer to it, and no
references to it. (There are, of course, exceptions, e.g. like
the return value of [] in an std::vector, where you actually
want to allow access to the element, and thus explicitly allow
the client to modify something internal to your data structure.)

If he drops the pointers:

void add( CNetwork newNetwork )
{
_netlist.push_back( newNetwork ) ;
}

and it works.

* * const CNetwork* getNetwork(CString netNameOrGuid)

The interface of this function is better if (a) it's 'const'
and (b) its argument is not passed by value:

Agreed with (a), but why (b). If CString has value semantics,
it should be treated as a value, and thus passed by value. (I
know the optimization argument, but until he has really
understood this distinction between values and entities, it's
definitely premature optimization.)

[...]

private:
* std::list<CNetwork*_netlist;
};

CNetworkList getNetworkList()
{
* * int i = 0;
* * HRESULT * * hResult;
* * CNetworkList netList;

*while( ConnMgrEnumDestinations( i, &connInfo ) == 0 )
* * * * {
* * * * * * CNetwork* pNetWork = new
CNetwork(CString(connInfo.szDescription), connInfo.guid);
* * * * * * if (pNetWork)

'new' never returns NULL. *You should, however, surround it
with 'try-catch' since 'new' throws 'std::bad_alloc' if
something happens.

Only if that something is running out of memory. There are a
lot of things you can do which result in undefined behavior.
(Witness this program.)

* * * * * * {
* * * * * * * * netList.Add(pNetWork);
* * * * * * }

* * * * * * i++;
* * * * }
}

When I call this code :
m_NetworkList = getNetworkList();

Where?

Just a guess, but probably in the destructor of the temporary
returned by getNetworkList(). At least, that's the first place
I see off hand with undefined behavior. Basically, he
constructed a list in the netList variable of getNetworkList;
this list assumed ownership of the CNetwork objects it
contained, and deletes them in its destructor (before
returning). Just before deleting them, however, it copies the
list into the temporary which will be returned. At that point,
you have two CNetworkList with the same pointers, which means
that when the destructor of the second one is called, bam.
Double delete, aka undefined behavior.

I got an assert in a Cstring desctructor so I suppose my
class is doing wrong... When I debug in step by step I
don't really understand the calls, it seems Clear() is
called why it shoudn't.

You call it in your destructor. You have a local variable of
CNetworkList type in your function, and you have a temporary
which is returned. That's two calls to Clear() that I see right
off.

Post complete code and provide a test driver that would
produce the network (instead of 'ConnMgrEnumDesitnations'
which C++ doesn't have).

That too. But I'd suggest he start by reading Scott Meyers.

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]

[Mosfet]

James Kanze a écrit :

On Oct 24, 4:43 pm, Victor Bazarov <[e-mail]:

>John Doe wrote:

>>I wrote a small class to enumerate available networks on a
smartphone :

>>class CNetwork

>Why do you need the 'C' in front of the name? I can
understand 'SP' (for smartphone), but 'C'?

I thought C was the prefix Microsoft reserved for MFC. With
namespaces, of course, you don't need such things at all
(although maybe the class should be in a SmartPhone namespace).

>>{
public:
CNetwork() {};
CNetwork(CString& netName, GUID netguid):
_netname(netName), _netguid(netguid) {}

>If your class owns the member '_netname' (which it probably
should, as you designed it), consider passing the initialiser
for it as a reference to const:

> CNetwork(CString const& netName, GUID...

Well, I suppose it really depends on what CString is. But I
think this one is from MFC, and that it has basic copy
semantics, much like std::string (which he should probably
consider using, instead of CString), then the most natural form
is call by value. Of course, if he doesn't use call by value or
a const reference, he can't construct from anything but an
lvalue---no CString returned from a function, for example.

>> ~CNetwork() {}

>> CString& getName() { return _netname; }

>This is a bad idea. You are exposing the innards of your
class to any change that you can't control or even know about.
If your 'Network' needs to report its name, this member has
to be 'const' and should return a reference to const:

> CString const& getName() cosnt { return _netname; }

Again, returning a value is more natural. In this case,
returning even a const reference is dangerous, because it only
really works if the client copies it immediately (in which case,
RVO would have eliminated the copy in return by value);
otherwise, you have a potential problem with the lifetime of the
reference.

A string is a value; it's almost always wrong to have a
reference or a pointer to one.

Of course, making the function const is a good idea.

>> GUID getGuid() { return _netguid; }

>Same here:

> GUID getGuid() const { return _netguid; }

>>private:
CString _netname;
GUID _netguid;
};

>>class CNetworkList
{
public:
typedef std::list<CNetwork*>::iterator NetworkListIt;

>Does this need to be public?

>> CNetworkList() {}

>> ~CNetworkList()
{
Clear();
}

>> CNetworkList::CNetworkList(const CNetworkList& rhs) {
CopyObj(rhs);
}

>What is that? Why couldn't you just use the normal copy
constructor form:

> CNetworkList::CNetworkList(const CNetworkList& rhs) :
netlist(rhs.netlist) {}

>?

Because the semantics aren't correct. His list is a list of
pointers (which is probably the real error), and he needs to
copy the pointed to objects, or strange things are goind to
happen.

> And if it matters, this is pretty much unnecessary because
the compiler will provide you with the copy constructor that
does exactly that.

>> CNetworkList& CNetworkList::operator=(const CNetworkList& rhs)
{
CopyObj(rhs);
return *this;
}

>Again, the assignment operator provided by the compiler will work just
fine, most likely. You don't have to provide your own.

>> void CopyObj(const CNetworkList& rhs)
{
_netlist = rhs._netlist;
}

Except, of course, he got it wrong here. Given the semantics of
his destructor, if he's using pointers, he needs something like:

_netlist.clear() ;
for ( NetworkListIt iter = rhs._netlist.begin() ;
iter != rhs._netlist.end() ;
++ iter ) {
_netlist.push_back( new CNetwork( **iter ) ) ;
}

Another good reason not to use pointers.

>> void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
}

With values, rather than pointers, this becomes simply:
_netlist.clear() ;

>> void Add(CNetwork* network)
{
_netlist.push_back(network);
}

>Now, you do realise that your list is made the owner of that
pointer here, right?

That's the real problem. It is as far as the destructor's
concerned. But not according to the copy constructor nor the
assignment operator. It's not surprising that he's having
problems.

Again, CNetwork is a value, not an entity (at least from what
little I can see here). That means: no pointer to it, and no
references to it. (There are, of course, exceptions, e.g. like
the return value of [] in an std::vector, where you actually
want to allow access to the element, and thus explicitly allow
the client to modify something internal to your data structure.)

If he drops the pointers:

void add( CNetwork newNetwork )
{
_netlist.push_back( newNetwork ) ;
}

and it works.

>> const CNetwork* getNetwork(CString netNameOrGuid)

>The interface of this function is better if (a) it's 'const'
and (b) its argument is not passed by value:

Agreed with (a), but why (b). If CString has value semantics,
it should be treated as a value, and thus passed by value. (I
know the optimization argument, but until he has really
understood this distinction between values and entities, it's
definitely premature optimization.)

[...]

>>private:
std::list<CNetwork*_netlist;
};

>>CNetworkList getNetworkList()
{
int i = 0;
HRESULT hResult;
CNetworkList netList;

>> while( ConnMgrEnumDestinations( i, &connInfo ) == 0 )
{
CNetwork* pNetWork = new
CNetwork(CString(connInfo.szDescription), connInfo.guid);
if (pNetWork)

>'new' never returns NULL. You should, however, surround it
with 'try-catch' since 'new' throws 'std::bad_alloc' if
something happens.

Only if that something is running out of memory. There are a
lot of things you can do which result in undefined behavior.
(Witness this program.)

>> {
netList.Add(pNetWork);
}

>> i++;
}
}

>>When I call this code :
m_NetworkList = getNetworkList();

>Where?

Just a guess, but probably in the destructor of the temporary
returned by getNetworkList(). At least, that's the first place
I see off hand with undefined behavior. Basically, he
constructed a list in the netList variable of getNetworkList;
this list assumed ownership of the CNetwork objects it
contained, and deletes them in its destructor (before
returning). Just before deleting them, however, it copies the
list into the temporary which will be returned. At that point,
you have two CNetworkList with the same pointers, which means
that when the destructor of the second one is called, bam.
Double delete, aka undefined behavior.

>>I got an assert in a Cstring desctructor so I suppose my
class is doing wrong... When I debug in step by step I
don't really understand the calls, it seems Clear() is
called why it shoudn't.

You call it in your destructor. You have a local variable of
CNetworkList type in your function, and you have a temporary
which is returned. That's two calls to Clear() that I see right
off.

>Post complete code and provide a test driver that would
produce the network (instead of 'ConnMgrEnumDesitnations'
which C++ doesn't have).

That too. But I'd suggest he start by reading Scott Meyers.

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]

Thanks.
Actually I am reading effective STL and there is a chapter where is
written :
Containers will make a copy of the object that you insert. This is fine
when inserting integers and strings, but for large/complex objects
becomes undesirable. If you do not want multiple copies of the object
floating around, store pointers in containers rather than actual
objects. If you are happy with multiple copies, get to know your
copy-constructor.
Usually I use object and not pointers but I told me that I could try
with pointers. OF course there are some issues with pointer ownerships
so I will do as I always do and will do some advanced tricks only in a
few years ;-)

[Thomas J. Gritzan]

John Doe schrieb:

Victor Bazarov wrote:

>John Doe wrote:

So here is the update code (without the try for new I will do it later)
but now I get an error :
error C2662: 'NetworkList::Clear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

so I have added const to Clear()...

But Clear changes the class, at least logically, so making Clear() const
is wrong. Why do you need the function anyway?

//============================================//
// NetworkManager.h
//============================================//
struct DeletePointer {
template<typename T>
void operator()(const T* ptr) const
{
delete ptr;
}
};

class Network
{
public:
Network() {};
Network(CString const& netName, GUID netguid):
_netname(netName), _netguid(netguid) {}

~Network() {}

You don't need to define an empty destructor.

CString const& getName() { return _netname; }

getName should be const.

GUID getGuid() const { return _netguid; }

private:
CString _netname;
GUID _netguid;
};
class NetworkList
{
typedef std::list<Network*>::iterator NetworkListIt;

public:

NetworkList()
{
}

~NetworkList()
{
Clear();
}

void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
}

Clear deletes the objects in your list, but you don't clear the list
itself. You have dangling pointers.

[...]

const Network* getNetwork(CLSID guid) const
{
if (!_netlist.empty())
Clear();

Here you call delete on all your pointers, but you don't clear _netlist.
After that, you use these dangling pointers, which is undefined
behaviour and most likely will crash your program.

Why do you clear the list just before you use it?

std::list<Network*>::const_iterator it;
//NetworkListIt it;
for (it = _netlist.begin(); it != _netlist.end(); ++it)
if ((*it)->getGuid() == guid)
return (*it);

return NULL;
}

private:
std::list<Network*_netlist;
};

Just as the other posters suggested, the easiest is not to use pointers
at all and change the member variable to:

std::list<Networknetlist; // std::vector would do it as well.

--
Thomas

[James Kanze]

On Oct 25, 12:00*am, Mosfet <[e-mail]:

James Kanze a écrit :

Actually I am reading effective STL and there is a chapter
where is written :
Containers will make a copy of the object that you insert.
This is fine when inserting integers and strings, but for
large/complex objects becomes undesirable. If you do not want
multiple copies of the object floating around, store pointers
in containers rather than actual objects. If you are happy
with multiple copies, get to know your copy-constructor.

Usually I use object and not pointers but I told me that I
could try with pointers. OF course there are some issues with
pointer ownerships so I will do as I always do and will do
some advanced tricks only in a few years ;-)

There is one idiom that might be applicable here: make the
contained objects immutable, and use boost::shared_ptr to manage
them. (Or better yet, develop your own invasive reference
counted pointer, based on e.g. the one in item 29 of "More
Effective C++".) This basically duplicates the way you'd handle
a value type in Java, with two restrictions: your value objects
can't contain pointers to other objects which might create
cycles, and it isn't thread safe. And it generally isn't worth
the bother, unless the objects are very, very expensive to copy
(not your case).

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]