From: Warren Young
Date: November 28 2006 4:51am
Subject: Re: [bug #7474] SSQLS compare operators patch
List-Archive: http://lists.mysql.com/plusplus/6157
Message-Id: <456BC067.7010109@etr-usa.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Korolyov Ilya wrote:
> Follow-up Comment #4, bug #7474 (project mysqlpp):
> 
> In current implementation, compare looks like
> 
>     int cmp; \ 
>     cmp = mysqlpp::sql_cmp(x.C1 , y.C1 ); \ 
>     if (cmp) return cmp; \ 
>     cmp = mysqlpp::sql_cmp(x.C2 , y.C2 );
> 
> etc
> 
> sql_cmp (custom.h) need general types, so it seems to me comparing for
> example 
> sql_datetime, lead to 2 converts sql_datetime=>std::string (correct me, if I
> 
> wrong). 
> 
> New implementation use 
> 
>   bool operator == (const NAME &other) const{\ 
>     bool cmp; \ 
>     cmp = (C1 == other.C1 ); \ 
>     if (cmp) return cmp; \ 
>     cmp = (C2 == other.C2 ); \ 
>     if (cmp) return cmp; \ 
>     return (C3 == other.C3);\ 
>  \ 
>   }
> 
> so, to compare sql_datetime, operator==() shoud be used without any 
> conversion. I guess it should be faster

This is a fine idea, but the patch is way too messy right now.  Several 
concerns come to mind while perusing it:

1. How do you justify leaving $parm2, $define, $set and $compp blank? 
They're still being used later on in compare.pl.  As far as I can tell, 
you have a choice between two courses.  First, you can completely remove 
the mechanisms that use these values, and then convince me that this is 
a reasonable choice.  I suspect you can remove all of sql_compare_*, but 
I haven't looked that deeply into it, so you'll have to make a good 
argument to get me to accept such a change.  The second option is to 
make a very conservative patch to just add your new operators, leaving 
the existing behavior untouched.  This makes the code bigger than it has 
to be, but it requires less justification.

2. I don't understand the reason for "bool cmp" in the == operator 
definition.  Why can't it be defined like the other two operators?

3. The initial assignment of $comp_equal is ugly: you're setting it to 
one of two different values based on a condition, but the assignments 
are separated by unrelated code.  This makes it hard to follow.  I'd say 
something like this instead:

	$comp_equal = ($i == 1 ? "    bool cmp; \\\n" : "");

4. It's a minor thing, but you've got some uneven use of whitespace. 
You'll inspire more confidence in me with neatly formatted code.  In my 
experience, messy code is associated with messy engineering.  You'll 
have an easier time convincing me of the soundness of your code if it 
looks good, too.