Add automatic type conversion to row::get() #1127
Conversation
zann1x
force-pushed
the
automatic-conversion
branch
4 times, most recently
from
46976e3
to
c0355f2
Compare
|
I honestly don't understand why the MSVC build is failing with |
There was a problem hiding this comment.
Thanks, the functionality provided by this PR definitely looks very useful.
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.
include/soci/type-holder.h
Outdated
| default: | ||
| break; |
There was a problem hiding this comment.
Shouldn't we throw here too?
In any case, please remove the default label to get the warnings here too.
There was a problem hiding this comment.
I don't see why this should throw as the constructor is private and it is only called during preparation for statement execution.
There was a problem hiding this comment.
This should be unreachable, right? I.e. it might be better to use std::unreachable() here but as long as we can't do this, throwing seems better than doing nothing.
include/soci/soci-platform.h
Outdated
| @@ -37,6 +37,9 @@ | |||
| //base class must have dll interface | |||
| #pragma warning(disable:4251 4275) | |||
|
|
|||
| // As long as we don't require C++17, we must disable the warning | |||
| // "conditional expression is constant" | |||
There was a problem hiding this comment.
Could you please add the context here, i.e. where would this warning be given?
There was a problem hiding this comment.
TBH I don't understand this error either. But as it didn't seem to be local to type-holder.h, I suppressed it here.
The failing build was this one: https://ci.appveyor.com/project/SOCI/soci/builds/49285927/job/fikvgmaiqq11a9np
There was a problem hiding this comment.
I guess the line refers to the val > static_cast<U>(t_max) test? If so, I can understand why the compiler would warn about it, as it could be always false for some types, but I'm not sure how to avoid it. Using pragma push/pop around this test might be better than suppressing the warning globally, however, it's a potentially useful one.
There was a problem hiding this comment.
Ah, I see. I'll move the pragma push/pop to this section.
include/soci/type-holder.h
Outdated
| { | ||
| static_assert(std::is_same<T, void>::value, "Unmatched raw type"); | ||
| // dummy value to satisfy the template engine, never used | ||
| static const db_type type = (db_type)0; | ||
| }; |
There was a problem hiding this comment.
Do we actually have to define it? I.e. could we just declare it without defining it?
include/soci/type-holder.h
Outdated
| static const db_type type = db_date; | ||
| }; | ||
|
|
||
| // Base class for storing type data instances in a container of holder objects |
There was a problem hiding this comment.
Is it actually "base"? It doesn't look like we inherit from it any longer.
If possible (i.e. if it works), I'd disable it for this function with |
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 |
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. |
I'm sorry but I really don't know/understand how to achieve that, so I would just leave it as is. |
|
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
force-pushed
the
automatic-conversion
branch
from
440d949
to
36baa80
Compare
zann1x
force-pushed
the
automatic-conversion
branch
from
36baa80
to
e14fe60
Compare
|
I rebased the branch onto main and added a commit to support movable types. 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. |
|
Thanks again! I'll merge this soon. |
Convert to the requested type if lossless conversion is possible to make the behaviour more compatible with the previous SOCI versions and more useful. See #1127.
|
Squash-merged now in the commit above. |
As discussed in #1088, it is possible that the merge of #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 #1097.