From: Warren Young
Date: November 25 2008 11:19am
Subject: Re: insertfrom( ) patch
List-Archive: http://lists.mysql.com/plusplus/8202
Message-Id: <492BDF4C.6090403@etr-usa.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Rick Gutleber wrote:
> 
> there is a difference in the 
> line-endings having to do with the jpeg examples in bmark.txt.

I guess it's just cgi_jpeg you're talking about.  The DOS-style line 
endings in its output are intentional.  HTTP uses DOS line endings in 
its headers, and a CGI program basically just builds an HTTP reply, so 
it has the same requirement.

We do need to suppress the silly bmark.txt diffs somehow.  We can do it 
by checking for -D, but I'd like to see if there a lighter hammer we can 
use first.  Basically, someone needs to look at a *hex* diff to see 
what's really going on at the byte level, to see if there's a way to 
satisfy CGI without creating diffs when moving between platforms.

> I took your suggested and moved the escape_string( ) functionality into 
> DBDriver

Okay.

> I gzipped the output so my e-mail client doesn't lunch the patch format 
> like it did last time.  Hopefully this will work for you.

It came through perfectly.

On to patch comments:

In dbdriver.h:

>  	/// \brief Executes the given query string
>  	///
>  	/// Wraps \c mysql_real_query() in the MySQL C API.
> +	///
> +	/// \sa escape_string(std::string*, const char*, size_t),
> +	/// escape_string_no_conn(char*, const char*, size_t)
>  	bool execute(const char* qstr, size_t length)
>  			{ return !mysql_real_query(&mysql_, qstr, length); }

Why the \sa here?

NoTransaction should be moved to transaction.h.  I don't see that 
anything in insertpolicy.h uses NoTransaction, so you don't need to 
replace it with an #include for transaction.h or a forward declaration.

In query.h:

> +#include "insertpolicy.h"

Is there a reason you're not including this within the Query public 
section, to add these classes to its definition?  I thought we'd agreed 
that these classes don't need to be at the top level of the mysqlpp 
namespace.

Later in query.h:

> +		for (Iter it = first; it != last; it++ ) {

Always preincrement iterators.  The difference doesn't matter for 
containers with simple pointer-like iterators -- vector, string, etc. -- 
as the compiler can optimize away the difference in increment forms just 
as it does for integers.  For more complex containers -- list, deque, 
set... -- postincrementing can be an efficiency sap, because compilers 
can't see that preincrementing is functionally identical in a for loop 
like this, so it can do away with the temporaries postincrementing 
requires.  The iterators are too complex for it to make such a leap.  I 
believe this is covered in one of Meyers' Effective C++ or STL books.

Speaking of iterators, have you tested the patch with an associative 
container?

And speaking of testing, what platforms have you tried this on?

Other than that... http://snipurl.com/6svzs

I need to sneak 3.0.8 out soon, to take care of the VC++ Connection 
errors and maybe one or two other things.  That done, I can apply your 
patch to kick off the v3.1 process.

You don't need to resubmit the patch.  You can just tell me what to do 
about these when I apply the patch.  But if you want, you can. :)

Also, is the anti-asplode patch coming separately?