From 4b137ed8693ee21d9f2bbcd104e83f312081b6e5 Mon Sep 17 00:00:00 2001 From: Andrey Semashev Date: Sat, 21 Jun 2014 20:54:11 +0400 Subject: [PATCH] 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 | 48 ++++++----- src/attribute_value_set.cpp | 17 ++-- test/run/attr_sets_insertion_lookup.cpp | 101 ++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 26 deletions(-) create mode 100644 test/run/attr_sets_insertion_lookup.cpp diff --git a/src/attribute_set_impl.hpp b/src/attribute_set_impl.hpp index 3a6f32b..2e687a3 100644 --- a/src/attribute_set_impl.hpp +++ b/src/attribute_set_impl.hpp @@ -276,27 +276,34 @@ public: 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 @@ public: // 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) { - case 1: // The erased element is the last one in the bucket + 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) + { + // 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)); diff --git a/src/attribute_value_set.cpp b/src/attribute_value_set.cpp index 6397f59..ac25cf5 100644 --- a/src/attribute_value_set.cpp +++ b/src/attribute_value_set.cpp @@ -391,27 +391,34 @@ private: 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; } diff --git a/test/run/attr_sets_insertion_lookup.cpp b/test/run/attr_sets_insertion_lookup.cpp new file mode 100644 index 0000000..f074825 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include + +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()); +}