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

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

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.

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

-----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-----

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

@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.

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

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