SOCI / soci

Official repository of the SOCI - The C++ Database Access Library
http://soci.sourceforge.net/
Boost Software License 1.0
1.37k stars 472 forks source link

Add automatic type conversion to row::get() #1127

Closed zann1x closed 3 months ago

zann1x commented 4 months ago

As discussed in https://github.com/SOCI/soci/issues/1088, it is possible that the merge of https://github.com/SOCI/soci/pull/1116 might require users to use a different data type when calling row::get<T>(...).

This PR tries to add implicit conversions between data types in the most conservative way. I.e., conversions are only defined between integer types. Unit tests that previously had to be changed to conform to the stricter type mapping are also reverted back to their old state.

The work here is based on https://github.com/SOCI/soci/pull/1097.

zann1x commented 4 months ago

I honestly don't understand why the MSVC build is failing with warning C4702: unreachable code because AFAICS these parts are actually used in the tests. Should I just disable C4702 for this file?

vadz commented 4 months ago

I honestly don't understand why the MSVC build is failing with warning C4702: unreachable code because AFAICS these parts are actually used in the tests. Should I just disable C4702 for this file?

If possible (i.e. if it works), I'd disable it for this function with #pragma push/pop, as usual.

zann1x commented 4 months ago

It's a bit of a pity to replace a type-safe type_holder template with a union, I'm not sure to understand why did it have to be done, I guess it's because template get() can't be virtual? But, in any case, if it has to be done like this, so be it.

In an eariler attempt to fixing this issue, I've had problems getting it to work with the old class hierarchy setup that isn't explicit about its db_type. I think it was a problem of having a templated get() which I wasn't able to combine with the templated type_holder::value(). As #1097 followed that same route, I honestly didn't think about an alternative way.

vadz commented 4 months ago

I've had problems getting it to work with the old class hierarchy setup that isn't explicit about its db_type. I think it was a problem of having a templated get() which I wasn't able to combine with the templated type_holder::value()

I think it should be possible to do it by having template function in the base (non-template) class using virtual functions overridden in the derived template class, but I really don't have time to try it, so I'm just leaving it here as an idea, but will merge it as is if you decide not to follow it.

BTW, forgot to say: @Sildra if you have any comments about this, please let us know if you have any comments about this.

zann1x commented 4 months ago

I think it should be possible to do it by having template function in the base (non-template) class using virtual functions overridden in the derived template class, but I really don't have time to try it, so I'm just leaving it here as an idea, but will merge it as is if you decide not to follow it.

I'm sorry but I really don't know/understand how to achieve that, so I would just leave it as is.

vadz commented 3 months ago

Sorry, it looks like this got conflicts due to the changes of #992, i.e. it needs to be adjusted to support movable types, such as blobs. Could you please check if this can be fixed? TIA!

zann1x commented 3 months ago

I rebased the branch onto main and added a commit to support movable types. The quickest solution I came up with was a separate get() method that returns a reference to the underlying value if and only if the types are matching. That was the easiest way to ensure that moving from that value is okay.

Unforunately, I don't have the time to create a more sophisticated solution at the moment. If there is more to do, it would be great if someone is willing to pick it up from here.

vadz commented 3 months ago

Thanks again! I'll merge this soon.

vadz commented 3 months ago

Squash-merged now in the commit above.