Return unique_ptr instead of pointer to deleted object

Coverity raised this issue. Test-Information: Code builds on macOS 10.12.4 and all unit tests pass; Swift runs fine. Change-Id: I8fb0805f6b2e0a21674ea32c0b1aee9e7b985639
author: Tobias Markmann <tm@ayena.de> 2017-03-30 07:32:28 (GMT)
committer: Tobias Markmann <tm@ayena.de> 2017-03-31 08:45:43 (GMT)
commit: 7663ca75731c4313dddbcde4d85f10383644a67a (patch)
tree: ecb1ed4c08b71bb58efc61980166e5064fefe68e /Swift
parent: 4d0391d824aaf94fbe152778581d51fecd588f6c (diff)
download: swift-7663ca75731c4313dddbcde4d85f10383644a67a.zip
swift-7663ca75731c4313dddbcde4d85f10383644a67a.tar.bz2
3 files changed, 29 insertions, 22 deletions
diff --git a/Swift/Controllers/Roster/GroupRosterItem.cpp b/Swift/Controllers/Roster/GroupRosterItem.cpp
index ac40afd..af5d0ca 100644
--- a/Swift/Controllers/Roster/GroupRosterItem.cpp
+++ b/Swift/Controllers/Roster/GroupRosterItem.cpp
@@ -1,40 +1,41 @@
 /*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2017 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
 
 #include <Swift/Controllers/Roster/GroupRosterItem.h>
+
+#include <memory>
+
 #include <boost/bind.hpp>
-//#include <boost/algorithm.hpp>
-#include <iostream>
 
 namespace Swift {
 
 GroupRosterItem::GroupRosterItem(const std::string& name, GroupRosterItem* parent, bool sortByStatus) : RosterItem(name, parent), sortByStatus_(sortByStatus), manualSort_(false) {
     expanded_ = true;
 }
 
 GroupRosterItem::~GroupRosterItem() {
 
 }
 
 void GroupRosterItem::setManualSort(const std::string& manualSortValue) {
     manualSort_ = true;
     bool changed = manualSortValue_ != manualSortValue;
     manualSortValue_ = manualSortValue;
     if (changed) {
         onChildrenChanged();
         onDataChanged();
     }
 }
 
 const std::string& GroupRosterItem::getSortableDisplayName() const {
     return manualSort_ ? manualSortValue_ : RosterItem::getSortableDisplayName();
 }
 
 bool GroupRosterItem::isExpanded() const {
     return expanded_;
 }
 
 /**
@@ -70,95 +71,93 @@ void GroupRosterItem::addChild(RosterItem* item) {
     onChildrenChanged();
     onDataChanged();
 }
 
 /**
  * Does not emit a changed signal.
  */
 void GroupRosterItem::removeAll() {
     std::vector<RosterItem*>::iterator it = children_.begin();
     displayedChildren_.clear();
     while (it != children_.end()) {
         ContactRosterItem* contact = dynamic_cast<ContactRosterItem*>(*it);
         if (contact) {
             delete contact;
         } else {
             GroupRosterItem* group = dynamic_cast<GroupRosterItem*>(*it);
             if (group) {
                 group->removeAll();
                 delete group;
             }
         }
         ++it;
     }
     children_.clear();
 }
 
 /**
  * Returns the removed item - but only if it's the only one, otherwise
  * the return result is undefined.
  */
-ContactRosterItem* GroupRosterItem::removeChild(const JID& jid) {
-    std::vector<RosterItem*>::iterator it = children_.begin();
-    ContactRosterItem* removed = nullptr;
+std::unique_ptr<ContactRosterItem> GroupRosterItem::removeChild(const JID& jid) {
+    auto it = children_.begin();
+    std::unique_ptr<ContactRosterItem> removed;
     while (it != children_.end()) {
         ContactRosterItem* contact = dynamic_cast<ContactRosterItem*>(*it);
         if (contact && contact->getJID() == jid) {
             displayedChildren_.erase(std::remove(displayedChildren_.begin(), displayedChildren_.end(), contact), displayedChildren_.end());
-            removed = contact;
-            delete contact;
+            removed = std::unique_ptr<ContactRosterItem>(contact);
             it = children_.erase(it);
             continue;
         }
         GroupRosterItem* group = dynamic_cast<GroupRosterItem*>(*it);
         if (group) {
-            ContactRosterItem* groupRemoved = group->removeChild(jid);
+            auto groupRemoved = group->removeChild(jid);
             if (groupRemoved) {
-                removed = groupRemoved;
+                removed = std::move(groupRemoved);
             }
         }
         ++it;
     }
     onChildrenChanged();
     onDataChanged();
     return removed;
 }
 
-GroupRosterItem* GroupRosterItem::removeGroupChild(const std::string& groupName) {
+std::unique_ptr<GroupRosterItem> GroupRosterItem::removeGroupChild(const std::string& groupName) {
     std::vector<RosterItem*>::iterator it = children_.begin();
-    GroupRosterItem* removed = nullptr;
+    std::unique_ptr<GroupRosterItem> removed;
     while (it != children_.end()) {
         GroupRosterItem* group = dynamic_cast<GroupRosterItem*>(*it);
         if (group && group->getDisplayName() == groupName) {
             displayedChildren_.erase(std::remove(displayedChildren_.begin(), displayedChildren_.end(), group), displayedChildren_.end());
-            removed = group;
-            delete group;
+            removed = std::unique_ptr<GroupRosterItem>(group);
             it = children_.erase(it);
             continue;
         }
         ++it;
     }
     onChildrenChanged();
     onDataChanged();
     return removed;
 }
 
 /**
  * Returns false if the list didn't need a resort
  */
 bool GroupRosterItem::sortDisplayed() {
     /* Not doing this until we import boost::algorithm*/
 //    if (boost::is_sorted(displayedChildren_begin(), displayedChildren_.end(), itemLessThan)) {
 //        return false;
 //    }
     //Sholudn't need stable_sort here
     std::sort(displayedChildren_.begin(), displayedChildren_.end(), sortByStatus_? itemLessThanWithStatus : itemLessThanWithoutStatus);
     return true;
 }
 
 bool GroupRosterItem::itemLessThanWithoutStatus(const RosterItem* left, const RosterItem* right) {
     return left->getSortableDisplayName() < right->getSortableDisplayName();
 }
 
 bool GroupRosterItem::itemLessThanWithStatus(const RosterItem* left, const RosterItem* right) {
     const ContactRosterItem* leftContact = dynamic_cast<const ContactRosterItem*>(left);
     const ContactRosterItem* rightContact = dynamic_cast<const ContactRosterItem*>(right);
diff --git a/Swift/Controllers/Roster/GroupRosterItem.h b/Swift/Controllers/Roster/GroupRosterItem.h
index a4e008f..ebb3849 100644
--- a/Swift/Controllers/Roster/GroupRosterItem.h
+++ b/Swift/Controllers/Roster/GroupRosterItem.h
@@ -1,50 +1,58 @@
 /*
  * Copyright (c) 2010-2016 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
 
 #pragma once
 
 #include <string>
 #include <vector>
 
 #include <Swift/Controllers/Roster/ContactRosterItem.h>
 #include <Swift/Controllers/Roster/RosterItem.h>
 
 namespace Swift {
 
 class GroupRosterItem : public RosterItem {
     public:
         GroupRosterItem(const std::string& name, GroupRosterItem* parent, bool sortByStatus);
         virtual ~GroupRosterItem();
+
         const std::vector<RosterItem*>& getChildren() const;
         const std::vector<RosterItem*>& getDisplayedChildren() const;
+
         void addChild(RosterItem* item);
-        ContactRosterItem* removeChild(const JID& jid);
-        GroupRosterItem* removeGroupChild(const std::string& group);
+        std::unique_ptr<ContactRosterItem> removeChild(const JID& jid);
+        std::unique_ptr<GroupRosterItem> removeGroupChild(const std::string& group);
         void removeAll();
+
         void setDisplayed(RosterItem* item, bool displayed);
-        boost::signals2::signal<void ()> onChildrenChanged;
-        static bool itemLessThanWithStatus(const RosterItem* left, const RosterItem* right);
-        static bool itemLessThanWithoutStatus(const RosterItem* left, const RosterItem* right);
         void setExpanded(bool expanded);
         bool isExpanded() const;
-        boost::signals2::signal<void (bool)> onExpandedChanged;
         void setManualSort(const std::string& manualSortValue);
         virtual const std::string& getSortableDisplayName() const;
+
+        boost::signals2::signal<void (bool)> onExpandedChanged;
+        boost::signals2::signal<void ()> onChildrenChanged;
+
+        static bool itemLessThanWithStatus(const RosterItem* left, const RosterItem* right);
+        static bool itemLessThanWithoutStatus(const RosterItem* left, const RosterItem* right);
+
     private:
         void handleChildrenChanged(GroupRosterItem* group);
         void handleDataChanged(RosterItem* item);
         bool sortDisplayed();
+
+    private:
         std::string name_;
         bool expanded_;
         std::vector<RosterItem*> children_;
         std::vector<RosterItem*> displayedChildren_;
         bool sortByStatus_;
         bool manualSort_;
         std::string manualSortValue_;
 };
 
 }
 
diff --git a/Swift/Controllers/Roster/Roster.cpp b/Swift/Controllers/Roster/Roster.cpp
index 9c9b9e3..f6f6ce0 100644
--- a/Swift/Controllers/Roster/Roster.cpp
+++ b/Swift/Controllers/Roster/Roster.cpp
@@ -137,64 +137,64 @@ struct JIDEqualsTo {
     JID jid;
 };
 
 void Roster::removeAll() {
     root_->removeAll();
     itemMap_.clear();
     onChildrenChanged(root_.get());
     onDataChanged(root_.get());
 }
 
 void Roster::removeContact(const JID& jid) {
     ItemMap::iterator item = itemMap_.find(fullJIDMapping_ ? jid : jid.toBare());
     if (item != itemMap_.end()) {
         std::vector<ContactRosterItem*>& items = item->second;
         items.erase(std::remove_if(items.begin(), items.end(), JIDEqualsTo(jid)), items.end());
         if (items.empty()) {
             itemMap_.erase(item);
         }
     }
     //Causes the delete
     root_->removeChild(jid);
 }
 
 void Roster::removeContactFromGroup(const JID& jid, const std::string& groupName) {
     std::vector<RosterItem*> children = root_->getChildren();
     std::vector<RosterItem*>::iterator it = children.begin();
     ItemMap::iterator itemIt = itemMap_.find(fullJIDMapping_ ? jid : jid.toBare());
     while (it != children.end()) {
         GroupRosterItem* group = dynamic_cast<GroupRosterItem*>(*it);
         if (group && group->getDisplayName() == groupName) {
-            ContactRosterItem* deleted = group->removeChild(jid);
+            auto deleted = group->removeChild(jid);
             if (itemIt != itemMap_.end()) {
                 std::vector<ContactRosterItem*>& items = itemIt->second;
-                items.erase(std::remove(items.begin(), items.end(), deleted), items.end());
+                items.erase(std::remove(items.begin(), items.end(), deleted.get()), items.end());
             }
         }
         ++it;
     }
 
     if (itemIt != itemMap_.end()) {
         for (auto* item : itemIt->second) {
             item->removeGroup(groupName);
         }
     }
 }
 
 
 void Roster::applyOnItems(const RosterItemOperation& operation) {
     if (operation.requiresLookup()) {
         applyOnItem(operation, operation.lookupJID());
     }
     else {
         applyOnAllItems(operation);
     }
 }
 
 void Roster::applyOnItem(const RosterItemOperation& operation, const JID& jid) {
     ItemMap::iterator i = itemMap_.find(fullJIDMapping_ ? jid : jid.toBare());
     if (i == itemMap_.end()) {
         return;
     }
     for (auto* item : i->second) {
         operation(item);
         filterContact(item, item->getParent());
author	Tobias Markmann <tm@ayena.de>	2017-03-30 07:32:28 (GMT)
committer	Tobias Markmann <tm@ayena.de>	2017-03-31 08:45:43 (GMT)
commit	7663ca75731c4313dddbcde4d85f10383644a67a (patch)
tree	ecb1ed4c08b71bb58efc61980166e5064fefe68e /Swift
parent	4d0391d824aaf94fbe152778581d51fecd588f6c (diff)
download	swift-7663ca75731c4313dddbcde4d85f10383644a67a.zip swift-7663ca75731c4313dddbcde4d85f10383644a67a.tar.bz2