Multiset fixes and more tests

libuavcan/include/uavcan/util/multiset.hpp

@@ -122,6 +122,11 @@ private:
 
     void compact();
 
+    enum RemoveStrategy { RemoveOne, RemoveAll };
+
+    template <typename Predicate>
+    void removeWhere(Predicate predicate, RemoveStrategy strategy);
+
     struct YesPredicate
     {
         bool operator()(const T&) const { return true; }
@@ -156,7 +161,7 @@ protected:
     { }
 #endif
 
-    /// Derived class destructor must call removeAll();
+    /// Derived class destructor must call clear();
     ~MultisetBase()
     {
         UAVCAN_ASSERT(getSize() == 0);
@@ -164,15 +169,11 @@ protected:
 
 public:
     /**
-     * This is needed for testing.
+     * Creates one item in-place and returns a pointer to it.
+     * If creation fails due to lack of memory, NULL will be returned.
+     * Complexity is O(N).
      */
-    enum { NumItemsPerDynamicChunk = Chunk::NumItems };
+    T* emplace()
-
-    /**
-     * Adds one item and returns a pointer to it.
-     * If add fails due to lack of memory, NULL will be returned.
-     */
-    T* add()
     {
         Item* const item = findOrCreateFreeSlot();
         if (item == NULL)
@@ -185,7 +186,7 @@ public:
     }
 
     template <typename P1>
-    T* add(P1 p1)
+    T* emplace(P1 p1)
     {
         Item* const item = findOrCreateFreeSlot();
         if (item == NULL)
@@ -198,7 +199,7 @@ public:
     }
 
     template <typename P1, typename P2>
-    T* add(P1 p1, P2 p2)
+    T* emplace(P1 p1, P2 p2)
     {
         Item* const item = findOrCreateFreeSlot();
         if (item == NULL)
@@ -211,7 +212,7 @@ public:
     }
 
     template <typename P1, typename P2, typename P3>
-    T* add(P1 p1, P2 p2, P3 p3)
+    T* emplace(P1 p1, P2 p2, P3 p3)
     {
         Item* const item = findOrCreateFreeSlot();
         if (item == NULL)
@@ -223,26 +224,22 @@ public:
         return item->ptr;
     }
 
-    /**
-     * @ref removeMatching()
-     */
-    enum RemoveStrategy { RemoveOne, RemoveAll };
-
     /**
      * Removes entries where the predicate returns true.
      * Predicate prototype:
      *  bool (T& item)
      */
     template <typename Predicate>
-    void removeMatching(Predicate predicate, RemoveStrategy strategy);
+    void removeAllWhere(Predicate predicate) { removeWhere<Predicate>(predicate, RemoveAll); }
 
     template <typename Predicate>
-    void removeAllMatching(Predicate predicate) { removeMatching<Predicate>(predicate, RemoveAll); }
+    void removeFirstWhere(Predicate predicate) { removeWhere<Predicate>(predicate, RemoveOne); }
 
-    template <typename Predicate>
+    void removeFirst(const T& ref) { removeFirstWhere(ComparingPredicate(ref)); }
-    void removeFirstMatching(Predicate predicate) { removeMatching<Predicate>(predicate, RemoveOne); }
 
-    void removeFirst(const T& ref) { removeFirstMatching(ComparingPredicate(ref)); }
+    void removeAll(const T& ref) { removeAllWhere(ComparingPredicate(ref)); }
+
+    void clear() { removeAllWhere(YesPredicate()); }
 
     /**
      * Returns first entry where the predicate returns true.
@@ -258,15 +255,11 @@ public:
         return const_cast<MultisetBase<T>*>(this)->find<Predicate>(predicate);
     }
 
-    /**
-     * Removes all items; all pool memory will be released.
-     */
-    void removeAll() { removeAllMatching(YesPredicate()); }
-
     /**
      * Returns an item located at the specified position from the beginning.
-     * Note that any insertion or deletion may greatly disturb internal ordering, so use with care.
+     * Note that addition and removal operations invalidate indices.
      * If index is greater than or equal the number of items, null pointer will be returned.
+     * Complexity is O(N).
      */
     T* getByIndex(unsigned index)
     {
@@ -280,7 +273,7 @@ public:
     }
 
     /**
-     * This is O(1)
+     * Complexity is O(N).
      */
     bool isEmpty() const { return find(YesPredicate()) == NULL; }
 
@@ -312,7 +305,7 @@ public:
         : MultisetBase<T>(static_, NumStaticEntries, allocator)
     { }
 
-    ~Multiset() { this->removeAll(); }
+    ~Multiset() { this->clear(); }
 
 #endif // !UAVCAN_TINY
 };
@@ -330,7 +323,7 @@ public:
 #endif
     { }
 
-    ~Multiset() { this->removeAll(); }
+    ~Multiset() { this->clear(); }
 };
 
 // ----------------------------------------------------------------------------
@@ -403,7 +396,7 @@ void MultisetBase<T>::compact()
 
 template <typename T>
 template <typename Predicate>
-void MultisetBase<T>::removeMatching(Predicate predicate, const RemoveStrategy strategy)
+void MultisetBase<T>::removeWhere(Predicate predicate, const RemoveStrategy strategy)
 {
     unsigned num_removed = 0;
 
@@ -416,13 +409,12 @@ void MultisetBase<T>::removeMatching(Predicate predicate, const RemoveStrategy s
             {
                 num_removed++;
                 static_[i].destroy();
+                if (strategy == RemoveOne)
+                {
+                    break;
+                }
             }
         }
-
-        if ((num_removed > 0) && (strategy == RemoveOne))
-        {
-            break;
-        }
     }
 #endif
 
@@ -443,6 +435,10 @@ void MultisetBase<T>::removeMatching(Predicate predicate, const RemoveStrategy s
                 {
                     num_removed++;
                     item.destroy();
+                    if (strategy == RemoveOne)
+                    {
+                        break;
+                    }
                 }
             }
         }

libuavcan/test/util/multiset.cpp

@@ -30,6 +30,25 @@ struct FindPredicate
     bool operator()(const std::string& value) const { return value == target; }
 };
 
+struct NoncopyableWithCounter : uavcan::Noncopyable
+{
+    static int num_objects;
+    long long value;
+
+    NoncopyableWithCounter() : value(0) { num_objects++; }
+    NoncopyableWithCounter(long long x) : value(x) { num_objects++; }
+    ~NoncopyableWithCounter() { num_objects--; }
+
+    static bool isNegative(const NoncopyableWithCounter& val)
+    {
+        return val.value < 0;
+    }
+
+    bool operator==(const NoncopyableWithCounter& ref) const { return ref.value == value; }
+};
+
+int NoncopyableWithCounter::num_objects = 0;
+
 
 TEST(Multiset, Basic)
 {
@@ -51,8 +70,8 @@ TEST(Multiset, Basic)
     ASSERT_FALSE(mset->getByIndex(10000));
 
     // Static addion
-    ASSERT_EQ("1", *mset->add("1"));
+    ASSERT_EQ("1", *mset->emplace("1"));
-    ASSERT_EQ("2", *mset->add("2"));
+    ASSERT_EQ("2", *mset->emplace("2"));
     ASSERT_EQ(0, pool.getNumUsedBlocks());
     ASSERT_EQ(2, mset->getNumStaticItems());
     ASSERT_EQ(0, mset->getNumDynamicItems());
@@ -62,11 +81,11 @@ TEST(Multiset, Basic)
     ASSERT_TRUE(*mset->getByIndex(1) == "2");
 
     // Dynamic addition
-    ASSERT_EQ("3", *mset->add("3"));
+    ASSERT_EQ("3", *mset->emplace("3"));
     ASSERT_EQ("3", *mset->getByIndex(2));
     ASSERT_EQ(1, pool.getNumUsedBlocks());
 
-    ASSERT_EQ("4", *mset->add("4"));
+    ASSERT_EQ("4", *mset->emplace("4"));
     ASSERT_LE(1, pool.getNumUsedBlocks());      // One or more
     ASSERT_EQ(2, mset->getNumStaticItems());
     ASSERT_EQ(2, mset->getNumDynamicItems());
@@ -111,7 +130,7 @@ TEST(Multiset, Basic)
     for (int i = 0; i < 100; i++)
     {
         const std::string value = toString(i);
-        std::string* res = mset->add(value);  // Will NOT override above
+        std::string* res = mset->emplace(value);  // Will NOT override above
         if (res == NULL)
         {
             ASSERT_LT(2, i);
@@ -127,10 +146,10 @@ TEST(Multiset, Basic)
 
     // Making sure there is true OOM
     ASSERT_EQ(0, pool.getNumFreeBlocks());
-    ASSERT_FALSE(mset->add("nonexistent"));
+    ASSERT_FALSE(mset->emplace("nonexistent"));
 
     // Removing odd values - nearly half of them
-    mset->removeAllMatching(oddValuePredicate);
+    mset->removeAllWhere(oddValuePredicate);
 
     // Making sure there's no odd values left
     for (unsigned kv_int = 0; kv_int <= max_value_integer; kv_int++)
@@ -170,8 +189,8 @@ TEST(Multiset, NoStatic)
     ASSERT_FALSE(mset->getByIndex(0));
 
     // Insertion
-    ASSERT_EQ("a", *mset->add("a"));
+    ASSERT_EQ("a", *mset->emplace("a"));
-    ASSERT_EQ("b", *mset->add("b"));
+    ASSERT_EQ("b", *mset->emplace("b"));
     ASSERT_LE(1, pool.getNumUsedBlocks());
     ASSERT_EQ(0, mset->getNumStaticItems());
     ASSERT_EQ(2, mset->getNumDynamicItems());
@@ -205,12 +224,12 @@ TEST(Multiset, PrimitiveKey)
     ASSERT_FALSE(mset->getByIndex(0));
 
     // Insertion
-    ASSERT_EQ(1, *mset->add(1));
+    ASSERT_EQ(1, *mset->emplace(1));
     ASSERT_EQ(1, mset->getSize());
-    ASSERT_EQ(2, *mset->add(2));
+    ASSERT_EQ(2, *mset->emplace(2));
     ASSERT_EQ(2, mset->getSize());
-    ASSERT_EQ(3, *mset->add(3));
+    ASSERT_EQ(3, *mset->emplace(3));
-    ASSERT_EQ(4, *mset->add(4));
+    ASSERT_EQ(4, *mset->emplace(4));
     ASSERT_EQ(4, mset->getSize());
 
 #if UAVCAN_CPP_VERSION >= UAVCAN_CPP11
@@ -223,3 +242,45 @@ TEST(Multiset, PrimitiveKey)
     ASSERT_FALSE(mset->getByIndex(1000));
 #endif
 }
+
+
+TEST(Multiset, NoncopyableWithCounter)
+{
+    using uavcan::Multiset;
+
+    static const int POOL_BLOCKS = 3;
+    uavcan::PoolAllocator<uavcan::MemPoolBlockSize * POOL_BLOCKS, uavcan::MemPoolBlockSize> pool;
+    uavcan::PoolManager<2> poolmgr;
+    poolmgr.addPool(&pool);
+
+    typedef Multiset<NoncopyableWithCounter, 2> MultisetType;
+    std::auto_ptr<MultisetType> mset(new MultisetType(poolmgr));
+
+    ASSERT_EQ(0, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(0,    mset->emplace()->value);
+    ASSERT_EQ(1, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(123,  mset->emplace(123)->value);
+    ASSERT_EQ(2, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(-456, mset->emplace(-456)->value);
+    ASSERT_EQ(3, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(456,  mset->emplace(456)->value);
+    ASSERT_EQ(4, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(-789, mset->emplace(-789)->value);
+    ASSERT_EQ(5, NoncopyableWithCounter::num_objects);
+
+    mset->removeFirst(NoncopyableWithCounter(0));
+    ASSERT_EQ(4, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(123, mset->getByIndex(0)->value);
+
+    mset->removeFirstWhere(&NoncopyableWithCounter::isNegative);
+    ASSERT_EQ(3, NoncopyableWithCounter::num_objects);
+    ASSERT_EQ(456, mset->getByIndex(1)->value);                 // -456 is now removed
+
+    mset->removeAllWhere(&NoncopyableWithCounter::isNegative);
+    ASSERT_EQ(2, NoncopyableWithCounter::num_objects);          // Only 1 and 2 are left
+
+    mset.reset();
+
+    ASSERT_EQ(0, pool.getNumUsedBlocks());
+    ASSERT_EQ(0, NoncopyableWithCounter::num_objects);          // All destroyed
+}