Fixed incorrect element insertion.

The inserted attributes and attribute values could sometimes be left not-findable by find() method and other methods that rely on it. The elements were still obtainable through iteration.

src/attribute_set_impl.hpp

@@ -276,27 +276,34 @@ struct attribute_set::implementation
         node* const n = m_Allocator.allocate(1, NULL);
         new (n) node(key, data);
 
+        node_list::iterator it;
         if (b.first == NULL)
         {
             // The bucket is empty
             b.first = b.last = n;
-            m_Nodes.push_back(*n);
+            it = m_Nodes.end();
+        }
+        else if (p == b.first)
+        {
+            // The new element should become the first element of the bucket
+            it = m_Nodes.iterator_to(*p);
+            b.first = n;
         }
         else if (p == b.last && key.id() > p->m_Value.first.id())
         {
             // The new element should become the last element of the bucket
-            node_list::iterator it = m_Nodes.iterator_to(*p);
+            it = m_Nodes.iterator_to(*p);
             ++it;
-            m_Nodes.insert(it, *n);
             b.last = n;
         }
         else
         {
             // The new element should be within the bucket
-            node_list::iterator it = m_Nodes.iterator_to(*p);
-            m_Nodes.insert(it, *n);
+            it = m_Nodes.iterator_to(*p);
         }
 
+        m_Nodes.insert(it, *n);
+
         return std::make_pair(iterator(n), true);
     }
 
@@ -309,24 +316,23 @@ struct attribute_set::implementation
 
         // Adjust bucket boundaries, if needed
         bucket& b = get_bucket(it->first.id());
-        unsigned int choice = (static_cast< unsigned int >(p == b.first) << 1) |
-            static_cast< unsigned int >(p == b.last);
-        switch (choice)
+        if (p == b.first)
+        {
+            if (p == b.last)
+            {
+                // The erased element is the only one in the bucket
+                b.first = b.last = NULL;
+            }
+            else
+            {
+                // The erased element is the first one in the bucket
+                b.first = value_traits::to_value_ptr(node_traits::get_next(b.first));
+            }
+        }
+        else if (p == b.last)
         {
-        case 1: // The erased element is the last one in the bucket
+            // The erased element is the last one in the bucket
             b.last = value_traits::to_value_ptr(node_traits::get_previous(b.last));
-            break;
-
-        case 2: // The erased element is the first one in the bucket
-            b.first = value_traits::to_value_ptr(node_traits::get_next(b.first));
-            break;
-
-        case 3: // The erased element is the only one in the bucket
-            b.first = b.last = NULL;
-            break;
-
-        default: // The erased element is somewhere in the middle of the bucket
-            break;
         }
 
         m_Nodes.erase_and_dispose(m_Nodes.iterator_to(*p), disposer(m_Allocator));

src/attribute_value_set.cpp

@@ -391,27 +391,34 @@ struct attribute_value_set::implementation
             p = new node(key, data, true);
         }
 
+        node_list::iterator it;
         if (b.first == NULL)
         {
             // The bucket is empty
             b.first = b.last = p;
-            m_Nodes.push_back(*p);
+            it = m_Nodes.end();
+        }
+        else if (where == b.first)
+        {
+            // The new element should become the first element of the bucket
+            it = m_Nodes.iterator_to(*where);
+            b.first = p;
         }
         else if (where == b.last && key.id() > where->m_Value.first.id())
         {
             // The new element should become the last element of the bucket
-            node_list::iterator it = m_Nodes.iterator_to(*where);
+            it = m_Nodes.iterator_to(*where);
             ++it;
-            m_Nodes.insert(it, *p);
             b.last = p;
         }
         else
         {
             // The new element should be within the bucket
-            node_list::iterator it = m_Nodes.iterator_to(*where);
-            m_Nodes.insert(it, *p);
+            it = m_Nodes.iterator_to(*where);
         }
 
+        m_Nodes.insert(it, *p);
+
         return p;
     }
 

test/run/attr_sets_insertion_lookup.cpp

@@ -0,0 +1,101 @@
+/*
+ *          Copyright Andrey Semashev 2007 - 2014.
+ * Distributed under the Boost Software License, Version 1.0.
+ *    (See accompanying file LICENSE_1_0.txt or copy at
+ *          http://www.boost.org/LICENSE_1_0.txt)
+ */
+/*!
+ * \file   attr_sets_insertion_lookup.cpp
+ * \author Andrey Semashev
+ * \date   21.06.2014
+ *
+ * \brief  This header contains tests for the attribute and attribute value sets. This test performs special checks
+ *         for insert() and find() methods that depend on the attribute name ids and the order in which
+ *         insert() operations are invoked, see https://sourceforge.net/p/boost-log/discussion/710022/thread/e883db9a/.
+ */
+
+#define BOOST_TEST_MODULE attr_sets_insertion_lookup
+
+#include <string>
+#include <sstream>
+#include <boost/config.hpp>
+#include <boost/test/unit_test.hpp>
+#include <boost/log/attributes/constant.hpp>
+#include <boost/log/attributes/attribute_set.hpp>
+#include <boost/log/attributes/attribute_value_set.hpp>
+
+namespace logging = boost::log;
+namespace attrs = logging::attributes;
+
+namespace {
+
+template< typename SetT, typename ValueT >
+void test_insertion_lookup(SetT& values, ValueT const& value)
+{
+    // Initialize attribute names. Each name will gain a consecutive id.
+    logging::attribute_name names[20];
+    for (unsigned int i = 0; i < sizeof(names) / sizeof(*names); ++i)
+    {
+        std::ostringstream strm;
+        strm << "Attr" << i;
+        names[i] = logging::attribute_name(strm.str());
+    }
+
+    // Insert attribute values in this exact order so that different cases in the hash table implementation are tested.
+    values.insert(names[17], value);
+    values.insert(names[1], value);
+    values.insert(names[8], value);
+    values.insert(names[9], value);
+    values.insert(names[10], value);
+    values.insert(names[16], value);
+    values.insert(names[0], value);
+    values.insert(names[11], value);
+    values.insert(names[12], value);
+    values.insert(names[13], value);
+    values.insert(names[14], value);
+    values.insert(names[15], value);
+    values.insert(names[18], value);
+    values.insert(names[19], value);
+    values.insert(names[4], value);
+    values.insert(names[5], value);
+    values.insert(names[7], value);
+    values.insert(names[6], value);
+    values.insert(names[2], value);
+    values.insert(names[3], value);
+
+    // Check that all values are accessible through iteration and find()
+    for (unsigned int i = 0; i < sizeof(names) / sizeof(*names); ++i)
+    {
+        BOOST_CHECK_MESSAGE(values.find(names[i]) != values.end(), "Attribute " << names[i] << " (id: " << names[i].id() << ") not found by find()");
+
+        bool found_by_iteration = false;
+        for (typename SetT::const_iterator it = values.begin(), end = values.end(); it != end; ++it)
+        {
+            if (it->first == names[i])
+            {
+                found_by_iteration = true;
+                break;
+            }
+        }
+
+        BOOST_CHECK_MESSAGE(found_by_iteration, "Attribute " << names[i] << " (id: " << names[i].id() << ") not found by iteration");
+    }
+}
+
+} // namespace
+
+BOOST_AUTO_TEST_CASE(attributes)
+{
+    logging::attribute_set values;
+    attrs::constant< int > attr(10);
+
+    test_insertion_lookup(values, attr);
+}
+
+BOOST_AUTO_TEST_CASE(attribute_values)
+{
+    logging::attribute_value_set values;
+    attrs::constant< int > attr(10);
+
+    test_insertion_lookup(values, attr.get_value());
+}