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(CStrin g& netName, GUID netguid):
_netname(netNam e), _netguid(netgui d) {}

~CNetwork() {}

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

private:
CString _netname;
GUID _netguid;
};


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

CNetworkList() {}

~CNetworkList()
{
Clear();
}

CNetworkList::C NetworkList(con st CNetworkList& rhs) {
CopyObj(rhs);
}

CNetworkList& CNetworkList::o perator=(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_b ack(network);
}

const CNetwork* getNetwork(CStr ing netNameOrGuid)
{
if ((netNameOrGuid .GetAt(0) == '{') &&
netNameOrGuid.G etLength() == 39)
{
CLSID guid;
if
(SUCCEEDED(CLSI DFromString(net NameOrGuid.GetB uffer(),&guid)) )
return getNetwork(guid );
}
else
{
NetworkListIt it;
for (it = _netlist.begin( ); it != _netlist.end(); ++it)
if (!(*it)->getName().Comp areNoCase(netNa meOrGuid))
return (*it);
}
return NULL;
}

const CNetwork* getNetwork(CLSI D guid)
{
if (!_netlist.empt y())
Clear();

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

return NULL;
}

private:
std::list<CNetw ork*_netlist;
};

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

while( ConnMgrEnumDest inations( i, &connInfo ) == 0 )
{
CNetwork* pNetWork = new
CNetwork(CStrin g(connInfo.szDe scription), connInfo.guid);
if (pNetWork)
{
netList.Add(pNe tWork);
}

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]

Re: What's wrong with this class

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(CStrin g& netName, GUID netguid):
_netname(netNam e), _netguid(netgui d) {}

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(CStrin g 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<CNetw ork*>::iterator NetworkListIt;

Does this need to be public?

>
CNetworkList() {}
>
~CNetworkList()
{
Clear();
}
>
CNetworkList::C NetworkList(con st CNetworkList& rhs) {
CopyObj(rhs);
}

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

CNetworkList::C NetworkList(con st CNetworkList& rhs) :
netlist(rhs.net list) {}

? 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::o perator=(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_b ack(network);
}

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

>
const CNetwork* getNetwork(CStr ing 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(CSti rng const& netNameOrGuid) const

{
if ((netNameOrGuid .GetAt(0) == '{') &&
netNameOrGuid.G etLength() == 39)
{
CLSID guid;
if
(SUCCEEDED(CLSI DFromString(net NameOrGuid.GetB uffer(),&guid)) )
return getNetwork(guid );
}
else
{
NetworkListIt it;
for (it = _netlist.begin( ); it != _netlist.end(); ++it)
if (!(*it)->getName().Comp areNoCase(netNa meOrGuid))
return (*it);
}
return NULL;
}
>
const CNetwork* getNetwork(CLSI D guid)

Same comment as above,

const CNetwork* getNetwork(CLSI D guid) const

{
if (!_netlist.empt y())
Clear();
>
NetworkListIt it;
for (it = _netlist.begin( ); it != _netlist.end(); ++it)
if ((*it)->getGuid() == guid)
return (*it);
>
return NULL;
}
>
private:
std::list<CNetw ork*_netlist;
};
>
CNetworkList getNetworkList( )
{
int i = 0;
HRESULT hResult;
CNetworkList netList;
>
while( ConnMgrEnumDest inations( i, &connInfo ) == 0 )
{
CNetwork* pNetWork = new
CNetwork(CStrin g(connInfo.szDe scription), 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(pNe tWork);
}
>
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 'ConnMgrEnumDes itnations' 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]

Re: What's wrong with this class

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::C lear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'

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


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

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

~Network() {}

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

private:
CString _netname;
GUID _netguid;
};


class NetworkList
{
typedef std::list<Netwo rk*>::iterator NetworkListIt;

public:

NetworkList()
{
}

~NetworkList()
{
Clear();
}

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

void Add(Network* network)
{
if (network)
_netlist.push_b ack(network);
}


const Network* getNetwork(CStr ing const& netNameOrGuid) const
{
if ((netNameOrGuid .GetAt(0) == '{') &&
netNameOrGuid.G etLength() == 39)
{
CLSID guid;
if (SUCCEEDED(CLSI DFromString((LP OLESTR)(LPCTSTR )netNameOrGuid, &guid)))
{
return getNetwork(guid );
}
}
else
{
std::list<Netwo rk*>::const_ite rator it;
//NetworkListIt it;
for (it = _netlist.begin( ); it != _netlist.end(); ++it)
if (!(*it)->getName().Comp areNoCase(netNa meOrGuid))
return (*it);
}
return NULL;
}

const Network* getNetwork(CLSI D guid) const
{
if (!_netlist.empt y())
Clear();

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

return NULL;
}

private:
std::list<Netwo rk*_netlist;
};

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

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

return netList;
}

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

void OnGettingNetwor kList()
{
NetworkList netList = getNetworkList( );
}

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

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

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

[Obnoxious User]

Re: What's wrong with this class

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::C lear' : 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 OnGettingNetwor kList()
{
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( _AtlInterlocked Decrement( &nRefs ) <= 0 ) {
pStringMgr->Free( this );
}
}

--
OU
Remember 18th of June 2008, Democracy died that afternoon.

http://frapedia.se/wiki/Information_in_English

[John Doe]

Re: What's wrong with this class

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::C lear' : 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 OnGettingNetwor kList()
>{
> 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 OnGettingNetwor kList()
{
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( _AtlInterlocked Decrement( &nRefs ) <= 0 ) {
> pStringMgr->Free( this );
> }
> }

>

[peter koch]

Re: What's wrong with this class

On 24 Okt., 18:14, John Doe <mos...@anonymo us.orgwrote:

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::C lear' : 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 OnGettingNetwor kList()
{
    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 OnGettingNetwor kList()
{
      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<Netwo rk*>). It
looks more natural to me to have it as a list of values
(std::list<Netw ork>). This also simplifies your design quite a lot.

/Peter

[Obnoxious User]

Re: What's wrong with this class

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::C lear' : 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 OnGettingNetwor kList()
>>{
>> 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]

Re: What's wrong with this class

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::C lear' : 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 OnGettingNetwor kList()
>>>{
>>> 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]

Re: What's wrong with this class

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::C lear' : 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 OnGettingNetwor kList()
>>>>{
>>>> 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.g etNetwork(lpAtt rVal);
const CString& strTmp = pNetwork->getName();

I get :

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

[peter koch]

Re: What's wrong with this class

On 24 Okt., 20:05, Mosfet <mos...@anonymo us.orgwrote:

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 OnGettingNetwor kList()
>>{
>>    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]

Re: What's wrong with this class

peter koch a écrit :

On 24 Okt., 20:05, Mosfet <mos...@anonymo us.orgwrote:

>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 OnGettingNetwor kList()
>>>>>{
>>>>> 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("Net work1"), guid));
netList.Add(new Network(_T("Net work2"), 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.g etNetwork(lpAtt rVal);
const std::wstring& strTmp = pNetwork->getName();

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

[Obnoxious User]

Re: What's wrong with this class

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

peter koch a écrit :

>On 24 Okt., 20:05, Mosfet <mos...@anonymo us.orgwrote:

>>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 OnGettingNetwor kList()
>>>>>>{
>>>>>> 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) {
>>...
>>>
>>}
>>>
>>getNetworkLis t(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("Net work1"), guid)); netList.Add(new
Network(_T("Net work2"), 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.g etNetwork(lpAtt rVal); const
std::wstring& strTmp = pNetwork->getName();
>
error C2662: 'Network::getNa me' : 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]

Re: What's wrong with this class

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 <mos...@anonymo us.orgwrote:
>>>Obnoxious User a écrit :
>>>>
>>>>
>>>>
>>>>On Fri, 24 Oct 2008 18:14:35 +0200, John Doe wrote:
>>>>>Obnoxiou s User wrote:
>>[snip]
>>>>>>How many times is this function called in your test case? Most
>>>>>>probabl y two times, right?
>>>>>>And what does it do each time?
>>>>>>[snip]
>>>>>>>void OnGettingNetwor kList()
>>>>>>>{
>>>>>>> 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) {
>>>...
>>>>
>>>}
>>>>
>>>getNetworkLi st(m_NetworkLis t);
>>>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("Net work1"), guid)); netList.Add(new
> Network(_T("Net work2"), 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.g etNetwork(lpAtt rVal); const
>std::wstring & strTmp = pNetwork->getName();
>>
>error C2662: 'Network::getNa me' : 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]

Re: What's wrong with this class

On Oct 24, 4:43 pm, Victor Bazarov <v.Abaza...@com Acast.netwrote:

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(CStrin g& netName, GUID netguid):
      _netname(netNam e), _netguid(netgui d) {}

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(CStrin g 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<CNetw ork*>::iterator NetworkListIt;

Does this need to be public?

    CNetworkList() {}

    ~CNetworkList()
    {
        Clear();
    }

    CNetworkList::C NetworkList(con st CNetworkList& rhs) {
        CopyObj(rhs);
    }

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

       CNetworkList::C NetworkList(con st CNetworkList& rhs) :
           netlist(rhs.net list) {}

?

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::o perator=(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.be gin() ;
iter != rhs._netlist.en d() ;
++ iter ) {
_netlist.push_b ack( 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_b ack(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_b ack( newNetwork ) ;
}

and it works.

    const CNetwork* getNetwork(CStr ing 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<CNetw ork*_netlist;
};

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

 while( ConnMgrEnumDest inations( i, &connInfo ) == 0 )
        {
            CNetwork* pNetWork = new
CNetwork(CStrin g(connInfo.szDe scription), 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(pNe tWork);
            }

            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 'ConnMgrEnumDes itnations'
which C++ doesn't have).

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

--
James Kanze (GABI Software) email:james.kan ze@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientier ter Datenverarbeitu ng
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

[Mosfet]

Re: What's wrong with this class

James Kanze a écrit :

On Oct 24, 4:43 pm, Victor Bazarov <v.Abaza...@com Acast.netwrote:

>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(CStrin g& netName, GUID netguid):
>> _netname(netNam e), _netguid(netgui d) {}

>

>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(CStrin g 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<CNetw ork*>::iterator NetworkListIt;

>

>Does this need to be public?

>

>> CNetworkList() {}

>

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

>

>> CNetworkList::C NetworkList(con st CNetworkList& rhs) {
>> CopyObj(rhs);
>> }

>

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

>

> CNetworkList::C NetworkList(con st CNetworkList& rhs) :
> netlist(rhs.net list) {}

>

>?

>
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::o perator=(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.be gin() ;
iter != rhs._netlist.en d() ;
++ iter ) {
_netlist.push_b ack( 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_b ack(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_b ack( newNetwork ) ;
}
>
and it works.
>

>> const CNetwork* getNetwork(CStr ing 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<CNetw ork*_netlist;
>>};

>

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

>

>> while( ConnMgrEnumDest inations( i, &connInfo ) == 0 )
>> {
>> CNetwork* pNetWork = new
>>CNetwork(CStr ing(connInfo.sz Description), 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(pNe tWork);
>> }

>

>> i++;
>> }
>>}

>

>>When I call this code :
>>m_NetworkLi st = 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 'ConnMgrEnumDes itnations'
>which C++ doesn't have).

>
That too. But I'd suggest he start by reading Scott Meyers.
>
--
James Kanze (GABI Software) email:james.kan ze@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientier ter Datenverarbeitu ng
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

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 ;-)