List:MySQL++« Previous MessageNext Message »
From:Chris Frey Date:February 14 2005 7:52pm
Subject:Re: [PATCH] mysql++ and strict compiler warnings
View as plain text  
On Mon, Feb 14, 2005 at 11:02:14AM -0700, Warren Young 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.

On second reading, this isn't needed, since the SQLQuery copy constructor
doesn't depend on stringstream's copy constructor, but passes in the source
str() into the stringstream constructor, so there is no copy danger here.

I still think the original warning is something weird in the C++ libraries.


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

Casting doesn't provide the precision comparison, and the existing comparison
code handles floats and doubles separately, so I don't believe there is any
possibility of mixing them.

Removing it is ok too, but it is used inside custom-macros.h, and it may
depend on it.

How about something like this:

template< int precision >
class sql_double {
	double val;
public:
	sql_double(float);	// implicit conversions
	sql_double(double);
	bool operator==(float) const;
	bool operator==(double) const;
	int compare(const sql_double &other) const;
};

template< int precision >
ostream& operator<< (ostream &os, const sql_double<precision> &d)
{
	// do precision output here
}

template< class comparable >
int sql_cmp(const comparable &a, const comparable &b)
{
	return a.compare(b);
}

sql_create_5
sql_create_5(stock,
        1,      // This number is used to make a SSQLS less-than-comparable.
                // If this number is n then if the first n elements are the 
                // same the two SSQLS are the same.  
                // In this case if two two stock's "item" are the same then
                // the two stock are the same.
        5,      // This number should generally be the same as the number of
                // elements in the list unless you have a good reason not to.
        string, item,
        longlong, num,
        double<5>, weight,	// higher precision for measurement
        double<2>, price,	// 2 digit money
        Date, sdate)

These classes could be overloaded as well to provide more accurate money
rounding in the comparison, etc.


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

I don't have a copy of the spec, but C99 has long long according to
http://www.kuro5hin.org/story/2001/2/23/194544/139
So I would hope there is some standard way to print this with printf().

C++ hasn't officially added this to the standard yet, but this is working its
way through the committee.  (Herb Sutter's report in CUJ's January issue)


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

I should perhaps do some performance test runs, but I think your point
about database speed being the bottleneck is good.  I think ostringstream
is the way to go.


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

Yes.  I figured it might be useful for future test compile runs.
It took me a while to research those flags. :-)

> >+// 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.

That's fine.  It was in the midst of compiler warnings for copy
constructors, and I was making a list of things not explicitly
initialized.


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

Thanks!  I'll try to split things out better.  This was all work done at once
while cycling through numerous compiles, so it all came out at once.

The guy who wrote the patches is in the best position to organize them though,
so I agree.

- Chris

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