On 23 March 2013 12:47, Vadim Zeitlin <[email protected]> wrote: > On Fri, 22 Mar 2013 14:40:36 +0000 Mateusz Loskot <[email protected]> wrote: > > ML> > The "s/unsigned/int/" fix for SQL_LONG is slightly more risky but, > still, > ML> > right now you simply get completely wrong values when reading from the > ML> > database (something like 4953.956 when the database contains -0.0013), > so > ML> > IMO it would still be better to apply it rather than not do it because > ML> > silently reading wrong data is one of the worst things that can happen > in > ML> > a database library. > > The above was correct too, in the "more risky" part. It turned out that > this fix did break Firebird tests[1] and that "unsigned" was used here to > work around the incorrect result when we casted 4000000000 (which is > greater than INT_MAX on 32 bit systems) to int and then to unsigned long > which could be 64 bits, yielding a wrong value. > > However I don't believe the original fix by Viacheslav (see 4c3cb629) > was the right way to do it, it only hid the problem but didn't solve it. > Using SQL_LONG for "unsigned long" simply can't work on LP64 systems > because SQL_LONG is just not big enough. So the real fix is to use > SQL_INT64 here and this is as simple as creating the column of right type > in the test database and this is what my fix[2] does.
Yes, I agree. The use of architecture-dependant integer types is a risky business in current version of SOCI. Only predictable sizes are will work in predictable way. I had to deal with similar bugs in ODBC recently. > ML> Indeed, we should aim to straighten the numeric types support as much > ML> as we can, within the extent of what we support now, I mean, without > ML> introducing new types yet. So, I'm glad you've got fix for that issue. > > Yes, I'm glad too because at one moment I was ready to totally despair... Yes, I imagine. > But finally it's not too bad and, provided that you use the correct column > types, things do seem to work now. That's the trick indeed. But it causes lots of inconvenience to users, and to ourselves too. > Still, the SQL to C++ types mapping is a huge mess and it clearly should > be the main focus of SOCI 4.0 release. For me, that is a must. > ML> > ML> Are you OK with releasing your fixes but without tests in 3.2.0? > ML> > > ML> > I prefer to commit them without tests to not committing them at all but > ML> > I'll try to add the tests too. Just please let me know until when do I > have > ML> > to do this. > ML> > ML> Let's say Sunday midnight :) > > This means we still have 36 hours for testing, :) > so I'd like to ask > everybody potentially affected by these issues to test the changes at > > https://github.com/SOCI/soci/pull/108 travis-ci did pretty good job and tested your fixes https://travis-ci.org/SOCI/soci/builds/5739508 > Please let me know if you see any unexpected results or have any remarks > or questions. TIA! It looks good to me. I've just merged your pull request into release/3.2.0 branch and the build of this branch is still green: https://travis-ci.org/SOCI/soci/builds/5751835 All good! > [1] Which I finally managed to run with embedded Firebird here, although > only using a dirty hack of creating a link to libfbembed.so called > libfbclient.so and putting it in LD_LIBRARY_PATH, i.e. in front of the > real libfbclient.so. I'd still like to have a proper fix at CMake level > but at least now I can run the tests which is already good. Yes, it would be good to improve it, I guess in FindFirebird.cmake. Vadim, thanks for the quick action! Ok, catching some sleep and working on releasing 3.2.0. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net ------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_d2d_mar _______________________________________________ soci-users mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/soci-users
