[
https://issues.apache.org/jira/browse/THRIFT-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17775188#comment-17775188
]
Mario Emmenlauer edited comment on THRIFT-5660 at 10/14/23 11:27 AM:
---------------------------------------------------------------------
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 not throw a {{{}std::bad_cast{}}}, because the value {{0}} can be
correctly represented (without under- or overflow) in both types. But it does
throw.
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}} (for example {{int32_t}} in my case). In order to perform the
comparison, C++ will promote one (or maybe even both?) types. Funny enough,
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 is to revert commit 6e9cbbd059. If somebody would volunteer
to implement a clean solution for an {{apache::thrift::numeric_cast()}} method,
I would be willing to test and help. But from my prior experience, I can say
that this is not a trivial task. It can easily lead to unforeseen culprits.
On a related note, I think the current implementation of
{{apache::thrift::numeric_cast()}} may have further problems. I.e. it 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. Currently an unsuspecting user may use it to cast also
floating types, in which case a precision-loss is not correctly detected.
was (Author: emmenlau):
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)
