Merge lp:~olafvdspek/drizzle/refactor11 into lp:~drizzle-trunk/drizzle/development

[Olaf van der Spek]

[Mark Atwood (fallenpegasus)]

Hi. Touching the atomics and the trait constructors needs to be reviewed by Brian. I'm asking him to review this before merging.

[Monty Taylor (mordred)]

I would be interested in an explanation of what your refactor is doing in the atomic classes. I'm not saying that it's wrong, but the template code in there isn't exactly friendly, so I'd like to know what it is you're shooting for here.

review: Needs Information

[Olaf van der Spek (olafvdspek)]

On Tue, Nov 15, 2011 at 7:48 PM, Monty Taylor <email address hidden> wrote:
> Review: Needs Information
>
> I would be interested in an explanation of what your refactor is doing in the atomic classes. I'm not saying that it's wrong, but the template code in there isn't exactly friendly, so I'd like to know what it is you're shooting for here.

I removed emptry constructors and destructors.
Moved atomic_base into atomic_impl.
Removed a type.

--
Olaf

[Monty Taylor (mordred)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/15/2011 05:04 PM, Olaf van der Spek wrote:
> On Tue, Nov 15, 2011 at 7:48 PM, Monty Taylor
> <email address hidden> wrote:
>> Review: Needs Information
>>
>> I would be interested in an explanation of what your refactor is
>> doing in the atomic classes. I'm not saying that it's wrong, but
>> the template code in there isn't exactly friendly, so I'd like to
>> know what it is you're shooting for here.
>
> I removed emptry constructors and destructors. Moved atomic_base
> into atomic_impl. Removed a type.

So - more specifically:

What are you accomplishing with the atomic_base -> atomic_impl move.
(not saying it's bad - I just want to know what you wanted to do here)

Why did you remove the empty constructors? They were preventing
default constructors from writing data to memory locations that we
didn't want them to write to?

Additionally - the pthread_atomics_test was there specifically to test
that the pthread-based atomic class worked, even if you happened to be
running the tests on a machine that happened to have real atomic
support. With the change to use the top-level atomic class, the test
no longer tests that.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7CvGEACgkQ2Jv7/VK1RgHwjwCg7Z+H8fbm8sVVA2T+EJjpEXgm
T2gAnRewAHJ/sEOshwZj1DlyXazdE02s
=aVWw
-----END PGP SIGNATURE-----

[Olaf van der Spek (olafvdspek)]

On Tue, Nov 15, 2011 at 8:28 PM, Monty Taylor <email address hidden> wrote:
> What are you accomplishing with the atomic_base -> atomic_impl move.
> (not saying it's bad - I just want to know what you wanted to do here)

Simpler code.

> Why did you remove the empty constructors? They were preventing
> default constructors from writing data to memory locations that we
> didn't want them to write to?

Eh, to what memory would default constructors write?

> Additionally - the pthread_atomics_test was there specifically to test
> that the pthread-based atomic class worked, even if you happened to be
> running the tests on a machine that happened to have real atomic
> support. With the change to use the top-level atomic class, the test
> no longer tests that.

Ah. Does it make sense to test code you're not going to use though?

--
Olaf

[Brian Aker (brianaker)]

@Olaf I'd leave the atomic implementation alone. This is an area where in the future someone could want what you are removing (and what has tests).

re: query_id
Right now it is one step closer to being encapsulated for catalogs, why remove that?

For if(), go on and put { } around what you expect it to execute. Over the years I've found just too many unintended bugs based on code getting shuffled, and no one realizing that the behavior of the branch had changed.

[Olaf van der Spek (olafvdspek)]

On Wed, Nov 16, 2011 at 10:08 AM, Brian Aker <email address hidden> wrote:
> @Olaf I'd leave the atomic implementation alone. This is an area where in the future someone could want what you are removing (and what has tests).

I'll restore the tests to test the pthread implementation.
But what removed code could someone want in the future?

> re: query_id
> Right now it is one step closer to being encapsulated for catalogs, why remove that?

How does it relate to catalogs?

Olaf

[lp:~olafvdspek/drizzle/refactor11]

2462. By Olaf van der Spek on 2011-11-22

Merge trunk

[Olaf van der Spek (olafvdspek)]

On Tue, Nov 15, 2011 at 8:37 PM, Olaf van der Spek <email address hidden> wrote:
> On Tue, Nov 15, 2011 at 8:28 PM, Monty Taylor <email address hidden> wrote:
>> What are you accomplishing with the atomic_base -> atomic_impl move.
>> (not saying it's bad - I just want to know what you wanted to do here)
>
> Simpler code.
>
>> Why did you remove the empty constructors? They were preventing
>> default constructors from writing data to memory locations that we
>> didn't want them to write to?
>
> Eh, to what memory would default constructors write?
>
>> Additionally - the pthread_atomics_test was there specifically to test
>> that the pthread-based atomic class worked, even if you happened to be
>> running the tests on a machine that happened to have real atomic
>> support. With the change to use the top-level atomic class, the test
>> no longer tests that.
>
> Ah. Does it make sense to test code you're not going to use though?

Monty?

--
Olaf

[lp:~olafvdspek/drizzle/refactor11]

2463. By Olaf van der Spek on 2011-12-16

Merge trunk

2464. By Olaf van der Spek on 2011-12-16

Merge trunk

Unmerged revisions

2464. By Olaf van der Spek on 2011-12-16

Merge trunk

2463. By Olaf van der Spek on 2011-12-16

Merge trunk

2462. By Olaf van der Spek on 2011-11-22

Merge trunk

2461. By Olaf van der Spek on 2011-11-12

Refactor

2460. By Olaf van der Spek on 2011-11-12

Refactor

2459. By Olaf van der Spek on 2011-11-12

Refactor

2458. By Olaf van der Spek on 2011-11-12

Refactor

=== modified file 'drizzled/atomic/gcc_traits.h'
--- drizzled/atomic/gcc_traits.h	2011-03-14 05:40:28 +0000
+++ drizzled/atomic/gcc_traits.h	2011-12-16 16:07:29 +0000
@@ -25,12 +25,9 @@
 template<typename T, typename D>
 class gcc_traits
 {
-
 public:
   typedef T value_type;
 
-  gcc_traits() {}
-
   inline value_type add_and_fetch(volatile value_type *value, D addend )
   {
     return __sync_add_and_fetch(value, addend);
@@ -91,4 +88,3 @@
 
 } /* namespace internal */
 } /* namespace drizzled */
-

=== modified file 'drizzled/atomic/pthread_traits.h'
--- drizzled/atomic/pthread_traits.h	2011-03-14 05:40:28 +0000
+++ drizzled/atomic/pthread_traits.h	2011-12-16 16:07:29 +0000
@@ -21,7 +21,6 @@
 #pragma once
 
 namespace drizzled {
-
 namespace internal {
 
 class mutex_wrapper
@@ -61,11 +60,8 @@
   mutex_wrapper my_lock;
 
 public:
-
   typedef T value_type;
 
-  pthread_traits() {}
-
   inline value_type add_and_fetch(volatile value_type *value, D addend )
   {
     my_lock.lock();
@@ -147,5 +143,3 @@
 
 } /* namespace internal */
 } /* namespace drizzled */
-
-

=== modified file 'drizzled/atomics.h'
--- drizzled/atomics.h	2011-08-14 17:04:01 +0000
+++ drizzled/atomics.h	2011-12-16 16:07:29 +0000
@@ -38,80 +38,67 @@
 namespace drizzled {
 namespace internal {
 
-template<typename I>            // Primary template
-struct atomic_base {
-  volatile I my_value;
-  atomic_base() : my_value(0) {}
-  virtual ~atomic_base() {}
-};
-
-template<typename I, typename D, typename T >
-class atomic_impl: private atomic_base<I>
+template<typename T, typename TR>
+class atomic_impl
 {
-  T traits;
+  TR traits;
+  volatile T my_value;
 public:
-  typedef I value_type;
-
-  atomic_impl() : atomic_base<I>(), traits() {}
-
-  value_type add_and_fetch( D addend )
+  typedef T value_type;
+
+  atomic_impl() : my_value(0) {}
+
+  value_type add_and_fetch( T addend )
   {
-    return traits.add_and_fetch(&this->my_value, addend);
+    return traits.add_and_fetch(&my_value, addend);
   }
 
-  value_type fetch_and_add( D addend )
+  value_type fetch_and_add( T addend )
   {
-    return traits.fetch_and_add(&this->my_value, addend);
+    return traits.fetch_and_add(&my_value, addend);
   }
 
   value_type fetch_and_increment()
   {
-    return traits.fetch_and_increment(&this->my_value);
+    return traits.fetch_and_increment(&my_value);
   }
 
   value_type fetch_and_decrement()
   {
-    return traits.fetch_and_decrement(&this->my_value);
+    return traits.fetch_and_decrement(&my_value);
   }
 
   value_type fetch_and_store( value_type value )
   {
-    return traits.fetch_and_store(&this->my_value, value);
+    return traits.fetch_and_store(&my_value, value);
   }
 
   bool compare_and_swap( value_type value, value_type comparand )
   {
-    return traits.compare_and_swap(&this->my_value, value, comparand);
+    return traits.compare_and_swap(&my_value, value, comparand);
   }
 
   operator value_type() const volatile
   {
-    return traits.fetch(&this->my_value);
-  }
-
-  value_type& _internal_reference() const
-  {
-    return this->my_value;
-  }
-
-protected:
-  value_type store_with_release( value_type rhs )
-  {
-    return traits.store_with_release(&this->my_value, rhs);
-  }
-
-public:
-  atomic_impl<I,D,T>& operator+=( D addend )
+    return traits.fetch(&my_value);
+  }
+
+  void store_with_release( value_type value )
+  {
+    traits.store_with_release(&my_value, value);
+  }
+
+  atomic_impl& operator+=( T addend )
   {
     increment(addend);
     return *this;
   }
 
-  atomic_impl<I,D,T>& operator-=( D addend )
+  atomic_impl& operator-=( T addend )
   {
     // Additive inverse of addend computed using binary minus,
     // instead of unary minus, for sake of avoiding compiler warnings.
-    return operator+=(D(0)-addend);
+    return operator+=(T(0)-addend);
   }
 
   value_type increment()
@@ -121,7 +108,7 @@
 
   value_type decrement()
   {
-    return add_and_fetch(D(-1));
+    return add_and_fetch(T(-1));
   }
 };
 
@@ -131,15 +118,16 @@
 /** See the Reference for details.
     @ingroup synchronization */
 template<typename T>
-struct atomic {
+struct atomic 
+{
 };
 
 /* *INDENT-OFF* */
-#define __DRIZZLE_DECL_ATOMIC(T)                                        \
-  template<> struct atomic<T>                                           \
-  : internal::atomic_impl<T,T,ATOMIC_TRAITS<T,T> > {                    \
-    atomic<T>() : internal::atomic_impl<T,T,ATOMIC_TRAITS<T,T> >() {}   \
-    atomic<T>& operator=( T rhs ) { store_with_release(rhs); return *this; } \
+#define __DRIZZLE_DECL_ATOMIC(T) \
+  template<> \
+  struct atomic<T> : internal::atomic_impl<T, ATOMIC_TRAITS<T, T> > \
+  { \
+    void operator=(T v) { store_with_release(v); } \
   };
 /* *INDENT-ON* */
 
@@ -164,11 +152,10 @@
 __DRIZZLE_DECL_ATOMIC(unsigned long long)
 #  else
 #   define __DRIZZLE_DECL_ATOMIC64(T)                                   \
-  template<> struct atomic<T>                                           \
-  : internal::atomic_impl<T,T,internal::pthread_traits<T,T> > {         \
-    atomic<T>()                                                         \
-      : internal::atomic_impl<T,T,internal::pthread_traits<T,T> >() {}  \
-    T operator=( T rhs ) { return store_with_release(rhs); }            \
+  template<> \
+  struct atomic<T> : internal::atomic_impl<T, internal::pthread_traits<T, T> > \
+  { \
+    void operator=(T v) { store_with_release(v); } \
   };
 __DRIZZLE_DECL_ATOMIC64(long long)
 __DRIZZLE_DECL_ATOMIC64(unsigned long long)
@@ -176,4 +163,3 @@
 /* *INDENT-ON* */
 
 }
-

=== modified file 'unittests/pthread_atomics_test.cc'
--- unittests/pthread_atomics_test.cc	2011-02-17 00:14:13 +0000
+++ unittests/pthread_atomics_test.cc	2011-12-16 16:07:29 +0000
@@ -21,44 +21,16 @@
 #include <config.h>
 
 #define BOOST_TEST_DYN_LINK
+
 #include <boost/test/unit_test.hpp>
-
-# if defined(__SUNPRO_CC)
-#  include <drizzled/atomic/sun_studio.h>
-# endif
-
-# if !defined(__ICC) && (defined(HAVE_GCC_ATOMIC_BUILTINS) || defined(__SUNPRO_CC))
-#  include <drizzled/atomic/gcc_traits.h>
-#  define ATOMIC_TRAITS internal::gcc_traits
-# else  /* use pthread impl */
-#  define ATOMIC_TRAITS internal::pthread_traits
-# endif
-
-#include <pthread.h>
-#include <drizzled/atomic/pthread_traits.h>
-
 #include <drizzled/atomics.h>
 
 using namespace drizzled;
 
-template<typename T>
-struct atomic_pthread {
-};
-
-#   define __DRIZZLE_DECL_ATOMIC_PTHREAD(T)                              \
-  template<> struct atomic_pthread<T>                                   \
-  : internal::atomic_impl<T,T, internal::pthread_traits<T,T> > {         \
-    atomic_pthread<T>()                                                 \
-      : internal::atomic_impl<T,T, internal::pthread_traits<T,T> >() {}  \
-    T operator=( T rhs ) { return store_with_release(rhs); }            \
-  };
-
-__DRIZZLE_DECL_ATOMIC_PTHREAD(unsigned int)
-
 BOOST_AUTO_TEST_SUITE(PthreadAtomicOperations)
 BOOST_AUTO_TEST_CASE(fetch_and_store)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
 
   BOOST_REQUIRE_EQUAL(0, u235.fetch_and_store(1));
 
@@ -70,7 +42,7 @@
 
 BOOST_AUTO_TEST_CASE(fetch_and_increment)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
 
   BOOST_REQUIRE_EQUAL(0, u235.fetch_and_increment());
   BOOST_REQUIRE_EQUAL(1, u235);
@@ -78,7 +50,7 @@
 
 BOOST_AUTO_TEST_CASE(fetch_and_add)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
 
   BOOST_REQUIRE_EQUAL(0, u235.fetch_and_add(2));
   BOOST_REQUIRE_EQUAL(2, u235);
@@ -86,7 +58,7 @@
 
 BOOST_AUTO_TEST_CASE(add_and_fetch)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
 
   BOOST_REQUIRE_EQUAL(10, u235.add_and_fetch(10));
   BOOST_REQUIRE_EQUAL(10, u235);
@@ -94,7 +66,7 @@
 
 BOOST_AUTO_TEST_CASE(fetch_and_decrement)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
 
   u235.fetch_and_store(15);
 
@@ -104,7 +76,7 @@
 
 BOOST_AUTO_TEST_CASE(compare_and_swap)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
 
   u235.fetch_and_store(100);
 
@@ -115,14 +87,14 @@
 
 BOOST_AUTO_TEST_CASE(increment)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
   u235.fetch_and_store(200);
   BOOST_REQUIRE_EQUAL(201, u235.increment());
 }
 
 BOOST_AUTO_TEST_CASE(decrement)
 {
-  atomic_pthread<uint32_t> u235;
+  atomic<uint32_t> u235;
   u235.fetch_and_store(200);
 
   BOOST_REQUIRE_EQUAL(199, u235.decrement());