List:MySQL++« Previous MessageNext Message »
From:Warren Young Date:February 14 2005 6:02pm
Subject:Re: [PATCH] mysql++ and strict compiler warnings
View as plain text  
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!
Thread
[PATCH] mysql++ and strict compiler warningsChris Frey7 Feb
  • Re: [PATCH] mysql++ and strict compiler warningsWarren Young14 Feb
    • Re: [PATCH] mysql++ and strict compiler warningsChris Frey14 Feb
      • Re: [PATCH] mysql++ and strict compiler warningsWarren Young15 Feb
        • Re: [PATCH] mysql++ and strict compiler warningsChris Frey15 Feb