Chris Frey wrote:
> - some weird one about base_ios<> not being initialized in the
> query class copy constructors... I'm not sure the iostreams
> library was meant to be copied, but mysql++ tries it anyway :-)
It's probably because SQLQuery (Query's parent) subclasses stringstream.
And indeed, one isn't supposed to do that, according to all C++
authorities I've ever read.
The right thing here is to give SQLQuery a stringstream member variable,
and then add the necessary functions to make SQLQuery look like a C++
stream object. This is a Wishlist item; you're welcome to tackle it.
> - a warning each for float and double comparisons with ==
> this is not guaranteed to succeed, since precision needs
> to be taken into account... it might be something to add to
> the wishlist, to have a specialized double and float comparison
> class which allows the programmer to specify precision
My first thought is that the Right Thing is to cast the float to double,
do the comparison, and let the result be converted to whatever type the
programmer wants. If they're mixing the two types, it is only
appropriate that we use as much precision as possible before giving the
result back to the user.
My second thought is to remove the feature entirely, because you
shouldn't be using == on floating point objects.
> - the snprintf() format flags were incorrect in sql_string.cpp,
> with the size flags in the wrong order (%dh instead of %hd, etc)
> Plus, some flags are not portable... there were "%uL" flags
> for ulonglong types, and I'm not sure where that came from,
> as it's not documented for linux or microsoft. I used
> the linux way of "%llu" for ulonglong in this patch.
> We may need to patch in "%I64u" for microsoft.
Bleh. And yet another variation for Borland C++, too, no doubt.
I wonder if the C99 standard says anything about this issue? I know
they did a lot of work adding portable sized-integer typedefs and such.
'Course, that begs the question of how many C++ compilers also support
the C99 extensions...
If there's no joy there, your offline suggestion of using ostreamstring
would seem the most reasonable way to attack this. I think worrying
about efficiency in this instance overlooks the overhead in communcating
with the database; the extra time taken isn't likely to even be
measurable in a real program. If a more efficient way is easy, by all
means, let's do it. But let's not add a bunch of conditional code to
add platform-specific efficient code where not helpful.
As a bonus, using ostreamstring fixes the _snprintf/snprintf portability
problem that I punted on a release or two ago. It also fixes, forever,
the buffer size problem we fixed before that. It may well be that
MySQL++ survives long enough to see the day that we get 128 bit integers.
> This is definitely pedantic territory, but I figure if someone wants to
> use this strict checking, mysql++ shouldn't stand in the way.
Indeed. I'm always in favor of removing warning-causing constructs, as
long as the workarounds are reasonable.
> Note: I've not included the auto-generated files
I would have yelled at you if you had. :)
> - return ColData(data[i].c_str(), res->types(i), is_nulls[i]);
> + return ColData(data.at(i).c_str(), res->types(i), is_nulls[i]);
This assumes Standard C++ exceptions exist, yet there's a legacy part of
MySQL++ that provides its own exceptions. If Standard exceptions exist
on all platforms we support, I have no problem with this patch. But if
not, we'll have to add our own out-of-bounds exception, and add that to
exceptions.h.in.
I've started a separate thread to discuss this issue.
> Index: software/mysql++/strict-config
> diff -u /dev/null software/mysql++/strict-config:1.1
> --- /dev/null Mon Feb 7 00:40:03 2005
> +++ software/mysql++/strict-config Mon Feb 7 00:21:52 2005
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +
> +export CXXFLAGS="-g -O2 -ansi -pedantic -Wall -W -Wold-style-cast -Wfloat-equal
> -Wwrite-strings -Woverloaded-virtual -Wno-long-long"
> +
> +./configure --enable-exception
> +
Did you intend this to be included?
> +// these are not copied?
> +//,parsed(), parsed_names(), parsed_nums()
I'm not sure what you mean by this comment. These members shouldn't
have to be in the initialization list, and there is no copying. Their
default ctors are called automatically to set up blank vectors. That's
it. I removed the comment.
One final, general comment: next time, please separate out patches with
different purposes. For instance, all the static_cast<> patches could
have been sent separately. It makes it easier to check the changes into
CVS, with good comments on each change.
Other than that, I have no problems with your patch. It's accepted, and
will appear in the next release. Thanks for your work!
