CppSQLite icon indicating copy to clipboard operation
CppSQLite copied to clipboard

Add getDoubleField API and update getFloatField API

Open EmosewaMC opened this issue 1 year ago • 2 comments

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;
}

EmosewaMC avatar Mar 06 '24 08:03 EmosewaMC

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;
}

EmosewaMC avatar Mar 06 '24 08:03 EmosewaMC

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

EmosewaMC avatar Mar 06 '24 21:03 EmosewaMC

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

Thanks for your contribution!

mqudsi avatar Apr 16 '24 21:04 mqudsi