List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:April 13 2011 10:45am
Subject:Re: bzr commit into mysql-trunk branch (davi:3348)
View as plain text  
Hi Davi.

Nice patch, only some nitpicks.

-- didrik

On Tue, Apr 12, 2011 at 9:50 PM, Davi Arnaut <davi.arnaut@stripped> wrote:

> # At a local mysql-trunk repository of davi
>
>  3348 Davi Arnaut       2011-04-12
>      Increment the I_P_List counter whenever a element is inserted into
>      the list. Previously, the counter would only be incremented if the
>      insertion method push_front() was used, in which case the counter
>      wouldn't be incremented if a element was inserted using the
> push_back()
>      and/or insert_after() methods.
>
>      Currently this does not affect the code base because there isn't any
>      code that uses a counted list with the push_back() or insert_after()
>      methods. Thus, add a unit test case to ensure that the counted list
>      works properly.
>
>    added:
>      unittest/gunit/sql_plist-t.cc
>    modified:
>      sql/sql_plist.h
>      unittest/gunit/CMakeLists.txt
> === modified file 'sql/sql_plist.h'
> --- a/sql/sql_plist.h   2011-04-11 11:52:24 +0000
> +++ b/sql/sql_plist.h   2011-04-12 19:50:44 +0000
> @@ -95,6 +95,7 @@ public:
>     *last= a;
>     *B::prev_ptr(a)= last;
>     I::set_last(B::next_ptr(a));
> +    C::inc();
>   }
>   inline void insert_after(T *pos, T *a)
>   {
> @@ -112,6 +113,7 @@ public:
>       }
>       else
>         I::set_last(B::next_ptr(a));
> +      C::inc();
>     }
>   }
>   inline void remove(T *a)
>
> === modified file 'unittest/gunit/CMakeLists.txt'
> --- a/unittest/gunit/CMakeLists.txt     2011-04-04 08:47:25 +0000
> +++ b/unittest/gunit/CMakeLists.txt     2011-04-12 19:50:44 +0000
> @@ -212,6 +212,7 @@ SET(TESTS
>   my_decimal
>   my_regex
>   sql_list
> +  sql_plist
>   thread_utils
>   )
>
>
> === added file 'unittest/gunit/sql_plist-t.cc'
> --- a/unittest/gunit/sql_plist-t.cc     1970-01-01 00:00:00 +0000
> +++ b/unittest/gunit/sql_plist-t.cc     2011-04-12 19:50:44 +0000
> @@ -0,0 +1,147 @@
> +/* Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
>  USA */
> +
> +// First include (the generated) my_config.h, to get correct platform
> defines,
> +// then gtest.h (before any other MySQL headers), to avoid min() macros
> etc ...
> +#include "my_config.h"
> +#include <gtest/gtest.h>
> +
> +#include "sql_plist.h"
> +
> +namespace {
> +
> +// A simple helper function to determine array size.
> +template <class T, int size>
> +int array_size(const T (&)[size])
> +{
> +  return size;
> +}
> +
> +// A simple helper function to insert values into a List.
> +template <class T, int size, class L>
> +void insert_values(T (&array)[size], L *list)
> +{
> +  uint ix, elements= list->elements();
> +  for (ix= 0; ix < size; ++ix)
> +    list->push_back(&array[ix]);
> +  EXPECT_EQ(ix + elements, list->elements());
> +}
> +
> +/*
> +  The fixture for testing the MySQL List and List_iterator classes.
> +  A fresh instance of this class will be created for each of the
> +  TEST_F functions below.
> +*/
> +class I_P_ListTest : public ::testing::Test
>

Please use CamelCase names for fixture classes and test names.


> +{
> +protected:
> +  I_P_ListTest()
> +    : m_int_list(), m_int_list_iter(m_int_list)
> +  {
> +  }
> +
> +public:
> +  template<typename V>
> +  struct I_P_ListTestValue
> +  {
> +    V value;
> +    I_P_ListTestValue(V val) : value(val) {}
>

(const V &val)


> +    bool operator == (const V val) const { return value == val; }
>

(const V &val)
actually: this operator==() is not needed.


> +    bool operator == (const I_P_ListTestValue<V> &obj) const
> +    { return value == obj.value; }
> +    struct I_P_ListTestValue<V> *next;
> +    struct I_P_ListTestValue<V> **prev;
> +  };
> +
> +protected:
> +  template<typename V>
> +  struct I_P_ListCountedPushBack
> +  {
> +    typedef I_P_ListTestValue<V> Value;
> +    typedef I_P_List<Value,
> +                     I_P_List_adapter<Value, &Value::next,
> &Value::prev>,
> +                     I_P_List_counter,
> +                     I_P_List_fast_push_back<Value>
> +                    > Type;
> +  };
> +
> +  I_P_ListCountedPushBack<int>::Type m_int_list;
> +  I_P_ListCountedPushBack<int>::Type::Iterator m_int_list_iter;
> +
> +private:
> +  // Declares (but does not define) copy constructor and assignment
> operator.
> +  GTEST_DISALLOW_COPY_AND_ASSIGN_(I_P_ListTest);
> +};
> +
> +
> +// Allow construction of test messages via the << operator.
> +template<typename T>
> +std::ostream &operator << (std::ostream &s, const
> I_P_ListTest::I_P_ListTestValue<T> &v)
>

long line


> +{
> +  return s << v.value;
> +}
> +
> +
> +// Tests that we can construct and destruct lists.
> +TEST_F(I_P_ListTest, ConstructAndDestruct)
>

CamelCase name, here and below.


> +{
> +  EXPECT_TRUE(m_int_list.is_empty());
> +  I_P_ListCountedPushBack<int>::Type *p_int_list;
> +  p_int_list= new I_P_ListCountedPushBack<int>::Type;
> +  EXPECT_TRUE(p_int_list->is_empty());
> +  delete p_int_list;
> +}
> +
> +
> +// Tests basic operations push and remove.
> +TEST_F(I_P_ListTest, BasicOperations)
> +{
> +  I_P_ListTestValue<int> v1(1), v2(2), v3(3);
> +  m_int_list.push_front(&v1);
> +  m_int_list.insert_after(&v1, &v2);
> +  m_int_list.push_back(&v3);
> +  EXPECT_FALSE(m_int_list.is_empty());
> +  EXPECT_EQ(3U, m_int_list.elements());
> +
> +  EXPECT_EQ(&v1, m_int_list.front());
> +  m_int_list.remove(&v1);
> +  EXPECT_EQ(&v2, m_int_list.front());
> +  m_int_list.remove(&v2);
> +  EXPECT_EQ(&v3, m_int_list.front());
> +  m_int_list.remove(&v3);
> +  EXPECT_TRUE(m_int_list.is_empty()) << "The list should be empty now!";
> +}
> +
> +
> +// Tests that we can iterate over values.
> +TEST_F(I_P_ListTest, Iterate)
> +{
> +  I_P_ListTestValue<int> values[]= {3, 2, 1};
> +  insert_values(values, &m_int_list);
> +  m_int_list_iter.init(m_int_list);
> +  for (int ix= 0; ix < array_size(values); ++ix)
> +  {
> +    EXPECT_EQ(values[ix], *m_int_list_iter++);
> +  }
> +  m_int_list_iter.init(m_int_list);
> +  I_P_ListTestValue<int> *value;
> +  int value_number= 0;
> +  while ((value= m_int_list_iter++))
> +  {
> +    EXPECT_EQ(values[value_number++], value->value);
> +  }
> +}
> +
> +}  // namespace
>
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>

Thread
bzr commit into mysql-trunk branch (davi:3348) Davi Arnaut12 Apr
Re: bzr commit into mysql-trunk branch (davi:3348)Tor Didriksen13 Apr