Re: Iterator destructor problem

From:

John Harrison <john_andronicus@hotmail.com>

Newsgroups:

comp.lang.c++

Date:

Mon, 25 Jun 2007 17:07:51 GMT

Message-ID:

<467FF6AE.8010409@hotmail.com>

Some corrections below

vivekian wrote:
vivekian wrote:

On Jun 19, 8:22 am, Obnoxious User <O...@127.0.0.1> wrote:

On Tue, 19 Jun 2007 12:34:31 +0000, vivekian wrote:

Hi,
I have this following class

[snip]

This is a snippet of code which uses this class :
        struct childInfo c ;
   c.relativeMeshId = atoi (meshId.substr(meshId.length(),1).c_str()) ;
   c.iChild = iNode ;
   (*pNode).second.children.push_back ((c)) ;

I'd guess you just cut this few lines out of a bigger context, since
neither 'iNode' nor 'pNode' are declared.

--
Obnoxious User

This is the code :

#include <map>
#include <string>
#include <vector>
#include <cstdlib>
#include <iostream.h>
#include <algorithm>

class nodeInfo ;

class childInfo
{
public:
    int relativeMeshId ;
    std::map <const std::string, nodeInfo >::iterator iChild ;
    childInfo () {};
    childInfo ( const childInfo & c ) ;
    childInfo & operator= (const childInfo&) ;
};

class nodeInfo
{
public:
    std::string meshId ;
    std::vector<childInfo> children;
    nodeInfo () {};
    nodeInfo (const nodeInfo & n ) ;
    nodeInfo & operator= (const nodeInfo&) ;
};

class NodeManager
{
private:
    std::map <const std::string , struct nodeInfo > _nodeList;
public:
    NodeManager();
    virtual ~NodeManager();

    void addRoute (const std::string parentMacAddr,const std::string
                                 childMacAddr,const std::string meshId);
} ;

NodeManager::NodeManager()
{
}

NodeManager::~NodeManager()
{
}

childInfo::childInfo ( const childInfo & c )
{
    relativeMeshId = c.relativeMeshId ;
    iChild = c.iChild ;
}

childInfo & childInfo::operator= (const childInfo & c)
{
    if ( this != &c )
    {
        relativeMeshId = c.relativeMeshId ;
        iChild = c.iChild ;
    }
    return *this ;
}

nodeInfo::nodeInfo (const nodeInfo & n)
{
    meshId = n.meshId ;
    copy (n.children.begin(), n.children.end(), children.begin()) ;
}

nodeInfo & nodeInfo::operator= (const nodeInfo &n)
{
    if (this != &n)
    {
        meshId = n.meshId;
        copy (n.children.begin(), n.children.end(), children.begin()) ;
    }
    return *this ;
}

void NodeManager::addRoute(const std::string pMacAddr,const
std::string cMacAddr,const std::string meshId)
{
    std::map <const std::string , struct nodeInfo>::iterator iNode =
_nodeList.find (cMacAddr) ;
    if (iNode == _nodeList.end())
    {
        struct nodeInfo n;
        n.meshId = meshId ;
        _nodeList[cMacAddr] = n;
        cout << "\nAdding " << n.meshId ;
    }
    else
    {
        //delete previous lineage
        //deleteRoute ( iNode -> first , iNode -> second.meshId ) ;
        iNode -> second.meshId = meshId ;
    }

    std::map <const std::string , struct nodeInfo>::iterator pNode =
_nodeList.find (pMacAddr);
    if (pNode != _nodeList.end())
    {
        cout << "\nAdding link to Parent " << pMacAddr ;
        childInfo c ;
        c.relativeMeshId = atoi (meshId.substr(meshId.length(),1).c_str()) ;
        c.iChild = iNode ;
        (*pNode).second.children.push_back ((c)) ;
    }
}

On Jun 19, 8:22 am, Obnoxious User <O...@127.0.0.1> wrote:

On Tue, 19 Jun 2007 12:34:31 +0000, vivekian wrote:

Hi,
I have this following class

[snip]

I'd guess you just cut this few lines out of a bigger context, since
neither 'iNode' nor 'pNode' are declared.

--
Obnoxious User

Your code has bugs.

First error is you write assignment operators and copy constructors when
they are not needed. This is a mistake.

More seriously this copy ctor is bugged

nodeInfo::nodeInfo (const nodeInfo & n)
{
meshId = n.meshId ;
copy (n.children.begin(), n.children.end(), children.begin()) ;
}

because you fail to allocate any space for the children vector.
Ironaically if you hadn't bothered to write this copy constructor, then
there would not have been a bug. Which is my first point, don't write
copy constructors or assignment operators when they are not needed, it's
asking for trouble. Assignment operator has simialar bug. As before, get
rid, problem goes away.

Third point, this line is bugged

c.relativeMeshId = atoi (meshId.substr(meshId.length(),1).c_str()) ;

perhaps you meant

c.relativeMeshId = atoi (meshId.substr(meshId.length()-1,1).c_str()) ;

john