Утечка ресурсов при создании объекта

Question

У меня есть следующий код для создания узла внутри графа.Я получаю ошибку утечки ресурса, когда я запускаю инструмент статической проверки (coverity).Буду признателен, если вы укажете, как улучшить код:

class node {   
   public :  
     explicit node(std::string& name) : m_name(name) { }  
     void setlevel(int level)  
     { m_level = level; }  
   private :    
     ...  
 }  
class graph {  
   public :  
      void populateGraph()  
      {  
         std::string nodeName = getNodeName();   
         /* I get error saying variable from new not freed or pointed-to in function  
            nc::node::node(const std::string...) */  
         node* NodePtr = new node(nodeName);  
         /* I get error saying variable NodePtr not freed or pointed-to in function  
            nc::node::setLevel(int) */   
         NodePtr->setLevel(-1);  
         if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())  
             m_name2NodeMap[nodeName] = NodePtr;  
         NodePtr = NULL;  
      }  
....  
private :  
  std::map< std::string, node*> m_name2NodeMap;   
}

Я подумал, что мне нужно delete NodePtr в populateGraph, но затем выпустил, что вызовет дескриптор узла (~node) и удалите узел из графика.Итак, я установил NodePtr=NULL, чтобы увидеть, помогает ли это, но это не так.

Answer 1

Я не знаком с скрытностью или точными правилами, которые он использует, но кажется, что у вас будет утечка памяти, если имя узла уже есть на карте.То есть, если тело вашего оператора if не выполнено, вы теряете указатель на память, которую вы только что выделили.Возможно, вы хотели что-то вроде:

if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())  
    m_name2NodeMap[nodeName] = NodePtr;  
else
    delete NodePtr;
NodePtr = NULL; 

Редактировать: так как я ответил почти одновременно с Daemin, позвольте мне добавить больше деталей:

Как упомянул ildjarn, вам также необходимо освободить техобъекты, которые попадают на карту, добавляя деструктор:

~graph()
{
    for( std::map< std::string, node*>::iterator i = m_name2NodeMap.begin(); 
         i != m_name2NodeMap.end(); ++i )
    {
        delete i->second;
    }
}

Для полноты я должен отметить, что:

1. Карта будет удалена автоматически после завершения деструктора, так какэто переменная-член.
2. Записи в карте узлов будут удаляться автоматически при удалении карты.
3. Ключи строк будут удаляться при удалении записей.

---

Предпочтительный способ справиться со сложным временем жизни объекта - использовать умный указатель.Например, boost :: shared_ptr или tr1 :: shared_ptr будут работать следующим образом.Примечание: у меня может не быть точного синтаксиса.

class node {   
    ...
}

class graph {  
    public :  
    void populateGraph()  
    {  
        std::string nodeName = getNodeName();   
        boost::shared_ptr< node > NodePtr( new node(nodeName) );
        NodePtr->setLevel(-1);  
        if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())  
            m_name2NodeMap[nodeName] = NodePtr;
    }  
    ....  
    private :  
        std::map< std::string, boost::shared_ptr<node> > m_name2NodeMap;   
    }
};

Посмотрите, как мы устранили деструктор и явные вызовы для удаления?Теперь объекты узла будут автоматически уничтожены, как и имена узлов.

На другом узле вы должны взглянуть на функцию std :: map :: insert , которая должна исключить это выражение if allвместе.

Answer 2

Что вам нужно сделать, это дать graph деструктор и внутри него delete все node* s в m_name2NodeMap. И, конечно, поскольку вам нужен деструктор, вам также нужен конструктор копирования и оператор присваивания копии, в противном случае вы гарантированно получите повреждение памяти.

Вам также понадобится предложение else для if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end()) до delete NodePtr;.

Answer 3

Другие уже рассмотрели проблему утечки. На самом деле утечек много, поэтому я даже не буду комментировать их все ... (populateGraph, ~Graph, Graph(Graph const&) и Graph& operator=(Graph const&) по крайней мере ...)

Я предпочитаю предложить простое решение, которое работает:

class Graph
{
public:
  void addNode(std::string name) {
    _nodes.insert(std::make_pair(name, Node(name));
  }

private:
  std::map<std::string, Node> _nodes;
};

Что здесь происходит?

• Динамическое распределение памяти не требуется, map может прекрасно содержать Node по значению, и у вас не будет никакой утечки таким образом.
• std::map::insert выполнит вставку только в том случае, если эквивалентного ключа уже нет, нет необходимости делать find + [] (что вдвое сложнее, так как вы дважды вычисляете место для хранения элемента)

Answer 4

Вы не освобождаете NodePtr, когда не добавляете его в список. Оператору if нужно другое, где вы delete NodePtr;

if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())
{
    m_name2NodeMap[nodeName] = NodePtr;
}
else
{
    delete NodePtr;
}
NodePtr = NULL;