Skip to content

Conversation

@EmosewaMC
Copy link
Contributor

I wanted to expand the API here to have a getFloatField alongside the getDoubleField so that a cast to the end type was done automatically instead of requiring the caller to do so.

Tested with the following program that the getFloatFields return the correct data
sqlite test db

CREATE TABLE test (fVal real);
insert into test values (1.11);
insert into test values (1.5);
insert into test values (2);
insert into test values (20000000000);
insert into test values (-20000000000);

program

#include "sqlite3.h"
#include "CppSQLite3.h"
#include <string>
#include <iomanip>
#include <iostream>
#include <cassert>

int main() {
	CppSQLite3DB conn;
	conn.open("test.sqlite");
	auto query = conn.execQuery("SELECT * FROM test");
	while (!query.eof()) {
		std::cout << std::setprecision(15) << query.getFloatField("fVal") << " " << query.getFloatField(0) << std::endl;
		assert(query.getFloatField("fVal") == query.getFloatField(0));
		query.nextRow();
	}
	return 0;
}

Wanted to expand the API here to have a getFloatField alongside a getDoubleField so that a cast to the end type was done automatically instead of requiring the caller to do so.

Tested with the following program that the getFloatFields return the correct data
@EmosewaMC EmosewaMC changed the title Add getFloatField API Add getDoubleField API and update getFloatField API Mar 6, 2024
@EmosewaMC
Copy link
Contributor Author

I noticed there is also an API for CPpSQLite3Table which would be missing the same API, so I have added the same thing there.
Test is as attached for that class.

#include "sqlite3.h"
#include "CppSQLite3.h"
#include <string>
#include <iomanip>
#include <iostream>
#include <cassert>

int main() {
	CppSQLite3DB conn;
	conn.open("test.sqlite");
	auto query = conn.execQuery("SELECT * FROM test");
	while (!query.eof()) {
		std::cout << std::setprecision(15) << query.getFloatField("fVal") << " " << query.getFloatField(0) << std::endl;
		assert(query.getFloatField("fVal") == query.getFloatField(0));
		query.nextRow();
	}
	auto table = conn.getTable("select * from test");
	for (int i = 0; i < table.numRows(); i++) {
		std::cout << std::setprecision(15) << table.getFloatField("fVal") << " " << table.getFloatField(0) << std::endl;
		assert(table.getFloatField("fVal") == table.getFloatField(0));
		table.setRow(i);
	}
	return 0;
}

Copy link
Member

@mqudsi mqudsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, this seems fair, as there are separate getIntField() and getInt64Field() functions.

The big problem that I see is that this is a backwards-incompatible change. Anyone updating existing code to an updated version of this library is going to be in for a surprise when they suddenly lose precision (store a double, call double d = query.getFloatField(0)) because the value gets narrowed to a float before being upcast back to a double since the return type of getFloatField() changed under them. I don't think it would even generate a warning with the most pedantic options enabled, since you have an explicit call to static_cast<float>(double).

It's rather unfortunate the original name was getFloatField() and not getDoubleField() but there's nothing we can do about that, obviously.

else
{
return atof(fieldValue(nField));
return static_cast<float>(atof(fieldValue(nField)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about platform availability, but atoff can be used instead of first converting to a double then narrowing that to a float. atoff is implemented as a call to strtodf plus a manipulation of errno (on applicable platforms).

If we can't confirm availability on the major platforms, casting is acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: It's basically not available anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate. I did look this up as I was confused why atof (I assume is to float) returned a double. Should I try for strtodf instead?

CppSQLite3.h Outdated
Comment on lines 225 to 229
float getFloatField(int nField, float fNullValue=0.0) const;
float getFloatField(const char* szField, float fNullValue=0.0) const;

double getDoubleField(int nField, double dNullValue=0.0f) const;
double getDoubleField(const char* szField, double dNullValue=0.0f) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value for fNullValue should be 0.0f with the f suffix while the default for dNullValue should be 0.0 without the f suffix. It looks like you have them swapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just mixed them around. I did add that but clearly just mixed where I added them to

@EmosewaMC
Copy link
Contributor Author

In principle, this seems fair, as there are separate getIntField() and getInt64Field() functions.

The big problem that I see is that this is a backwards-incompatible change. Anyone updating existing code to an updated version of this library is going to be in for a surprise when they suddenly lose precision (store a double, call double d = query.getFloatField(0)) because the value gets narrowed to a float before being upcast back to a double since the return type of getFloatField() changed under them. I don't think it would even generate a warning with the most pedantic options enabled, since you have an explicit call to static_cast<float>(double).

It's rather unfortunate the original name was getFloatField() and not getDoubleField() but there's nothing we can do about that, obviously.

That is a fair point. I saw a video recently mentioning how making backwards incompatible changes basically sets the updatabiloty of a project back eons. If there isn't a way to fix this without breaking backwards compatibility I'm fine with closing this, just wanted to upstream the code from a personal project so others could use it. Appreciate the quick review

@mqudsi mqudsi merged commit 108a7f4 into neosmart:master Apr 16, 2024
@mqudsi
Copy link
Member

mqudsi commented Apr 16, 2024

After much reflection, I've decided to merge this change with a clear warning in the commit log.

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants