AVRO-4105: [C++] Remove boost::lexical_cast and boost::math #3277
Conversation
lang/c++/impl/json/JsonIO.hh
Outdated
| std::ostringstream oss; | ||
| if (boost::math::isfinite(t)) { | ||
| oss << boost::lexical_cast<std::string>(t); | ||
| } else if (boost::math::isnan(t)) { | ||
| if (std::isfinite(t)) { | ||
| oss << t; |
There was a problem hiding this comment.
Does std::locale::global affect the double-to-string formatting here; for example, can it output "12,34" with a comma, which would be invalid in JSON?
There was a problem hiding this comment.
Yes, I think so. Perhaps using std::to_chars is better?
There was a problem hiding this comment.
OK, charconv is unavailable on ARM CI: https://github.com/apache/avro/actions/runs/12558161302/job/35011882297?pr=3277
There was a problem hiding this comment.
It looks like the std::to_chars functions that take an std::chars_format parameter are available in GCC 11 (changes) but the ARM CI has 9.4.0.
oss.imbue(std::locale::classic()) then? Does Avro have tests in which the C++ global locale is not classic?
There was a problem hiding this comment.
I did a quick check and it seems there isn't any test case like that.
There was a problem hiding this comment.
It appears the lexical_cast also has the same locale based behavior. Given that there is no clean solution for C++14, and to_string is no worse than lexical_cast should we proceed with this PR? I'll merge this PR if no objection is forthcoming for a day or two.
wgtmac
force-pushed
the
remove_boost_math
branch
from
30db22b to
0e0d315
Compare
lang/c++/impl/json/JsonIO.hh
Outdated
| sep2(); | ||
| } | ||
|
|
||
| void encodeNumber(float t) { |
There was a problem hiding this comment.
Since this is a specialization, it'll better to be explicit about it. Please add: template <> before this line.
lang/c++/impl/json/JsonIO.hh
Outdated
| @@ -404,9 +404,25 @@ public: | |||
|
|
|||
| template<typename T> | |||
| void encodeNumber(T t) { | |||
There was a problem hiding this comment.
Instead of having a template function encodeNumber and its specializations, it'll be cleaner to have (a) a template function numberToString and its specialization to convert the given number into a string and then (b) have a single function encodeNumber that puts the string into out_.
lang/c++/impl/json/JsonIO.hh
Outdated
| @@ -415,9 +431,10 @@ public: | |||
| void encodeNumber(double t) { | |||
There was a problem hiding this comment.
We can have a single specialization for float and double:
template<typename T>
typename std::enable_if<std::is_floating_point<T>::value, void>::type
encodeNumber(T t) {
// implementation common to float/double
}
template<typename T>
typename std::enable_if<!std::is_floating_point<T>::value, void>::type
encodeNumber(T t) {
// implementation for all other types
}
There was a problem hiding this comment.
Done. Please take a look again. Thanks!
wgtmac
force-pushed
the
remove_boost_math
branch
from
e7fe237 to
4c5a9ba
Compare
What is the purpose of the change
Remove boost::lexical_cast and boost::math
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation