[
https://issues.apache.org/jira/browse/THRIFT-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17775188#comment-17775188
]
Mario Emmenlauer commented on THRIFT-5660:
------------------------------------------
I think this is a breaking change :(
With this change in place, I can not successfully use the transport {{HEADER}}
in C++ anymore. It fails with a {{std::bad_cast}} in
{{{}lib/cpp/src/thrift/numeric_cast.h{}}}. The failing invocation is obviously
wrong when looking at the arguments. But it is far from trivial to identify the
culprit in the code, because of the automatic type promotion of C++.
The failing arguments are:
{code:java}
unsigned long i = 0;
return apache::thrift::numeric_cast<int32_t>(i);{code}
This should obviously not throw a {{{}std::bad_cast{}}}, but it does.
I've tried to understand where the current implementation is wrong. Without
spending significant time, this is not trivial. My current hunch is that the
following lines are problematic:
{code:java}
if (positive_overflow_possible && value > DstLim::max()) {{code}
and
{code:java}
if (negative_overflow_possible && (value < DstLim::lowest())) {{code}
One problem can be that {{value}} is of type {{Src}} (for example {{unsigned
long}} in my case), whereas {{DstLim::max()}} and {{DstLim::lowest()}} are of
type {{{}Dst{}}}. To perform the comparison, C++ will promote one or both
types. This is actually exactly the thing that the code should prohibit,
because it is unclear if type promotion (or casting) works correctly for the
given types.
The simple solution to fix the {{HEADER}} transport is to revert commit
6e9cbbd059. If somebody would indeed volunteer to implement a clean solution
for a {{apache::thrift::numeric_cast()}} method, I would be willing to test and
help, but from prior experience I can say that this is not a trivial task and
can easily lead to unforeseen culprits.
On a related note, I think the current implementation may not be safe to use
for non-integral types, or at least it would mostly work by chance, not by
design. There should be a {{static_assert}} or {{enable_if}} to prohibit wrong
usage.
> TTransportException: create thrift::numeric_cast
> ------------------------------------------------
>
> Key: THRIFT-5660
> URL: https://issues.apache.org/jira/browse/THRIFT-5660
> Project: Thrift
> Issue Type: Sub-task
> Components: C++ - Library
> Reporter: Christopher Friedt
> Assignee: Christopher Friedt
> Priority: Trivial
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> This adds an equivalent implementation of `boost::numeric_cast` written
> purely in standard c++.
> The implementation is relatively trivial and reduces the dependency on
> `boost`.
> Adapted from
> https://stackoverflow.com/a/49658950/5636218
> PR is here:
> [https://github.com/apache/thrift/pull/2689]
> See also:
> [https://github.com/zephyrproject-rtos/gsoc-2022-thrift/issues/147]
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
