emmenlau commented on a change in pull request #2502:
URL: https://github.com/apache/thrift/pull/2502#discussion_r783238054
##########
File path: compiler/cpp/src/thrift/generate/t_erl_generator.cc
##########
@@ -389,27 +390,36 @@ void t_erl_generator::close_generator() {
const std::string emit_double_as_string(const double value) {
std::stringstream double_output_stream;
- // sets the maximum precision:
http://en.cppreference.com/w/cpp/io/manip/setprecision
- // sets the output format to fixed:
http://en.cppreference.com/w/cpp/io/manip/fixed (not in scientific notation)
- double_output_stream <<
std::setprecision(std::numeric_limits<double>::digits10 + 1);
-
- #ifdef _MSC_VER
- // strtod is broken in MSVC compilers older than 2015, so std::fixed
fails to format a double literal.
- // more details: https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
- //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
- // and
- //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
- #if _MSC_VER >= MSC_2015_VER
- double_output_stream << std::fixed;
- #else
- // note that if this function is called from the erlang generator
and the MSVC compiler is older than 2015,
- // the double literal must be output in the scientific format. There
can be some cases where the
- // mantissa of the output does not have fractionals, which is
illegal in Erlang.
- // example => 10000000000000000.0 being output as 1e+16
- double_output_stream << std::scientific;
- #endif
+
+ #if defined(_MSC_VER) && _MSC_VER < MSC_2015_VER
+ // strtod is broken in MSVC compilers older than 2015, so std::fixed fails
to format a double literal.
+ // more details: https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
+ //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
+ // and
+ //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
+ // note that if this function is called from the erlang generator and the
MSVC compiler is older than 2015,
+ // the double literal must be output in the scientific format. There can
be some cases where the
+ // mantissa of the output does not have fractionals, which is illegal in
Erlang.
+ // example => 10000000000000000.0 being output as 1e+16
+ double_output_stream << std::scientific <<
std::setprecision(std::numeric_limits<double>::digits10 + 1);
#else
- double_output_stream << std::fixed;
+ //question from [email protected]: Does anybody know why we avoid
scientific notation here?
+ double_output_stream << std::fixed;
+ #if 0 //maintain compatibility with previous versions of
emit_double_as_string that could generate extra trailing digits
+ const int min_precision = std::numeric_limits<double>::digits10 + 1;
+ #else //don't add unneeded 0's
+ const int min_precision = 1;
+ #endif
+ if (std::isfinite(value) && value != 0.0) {
Review comment:
Rather than this check of `value != 0.0`, would it be better to check
the value of `static_cast<int>(floor(log10(abs(value)))`? Its numerically not
very stable to perform math with values very close to zero, so evaluating the
*result* of the math may be better than the source?
##########
File path: compiler/cpp/src/thrift/generate/t_erl_generator.cc
##########
@@ -389,27 +390,36 @@ void t_erl_generator::close_generator() {
const std::string emit_double_as_string(const double value) {
std::stringstream double_output_stream;
- // sets the maximum precision:
http://en.cppreference.com/w/cpp/io/manip/setprecision
- // sets the output format to fixed:
http://en.cppreference.com/w/cpp/io/manip/fixed (not in scientific notation)
- double_output_stream <<
std::setprecision(std::numeric_limits<double>::digits10 + 1);
-
- #ifdef _MSC_VER
- // strtod is broken in MSVC compilers older than 2015, so std::fixed
fails to format a double literal.
- // more details: https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
- //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
- // and
- //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
- #if _MSC_VER >= MSC_2015_VER
- double_output_stream << std::fixed;
- #else
- // note that if this function is called from the erlang generator
and the MSVC compiler is older than 2015,
- // the double literal must be output in the scientific format. There
can be some cases where the
- // mantissa of the output does not have fractionals, which is
illegal in Erlang.
- // example => 10000000000000000.0 being output as 1e+16
- double_output_stream << std::scientific;
- #endif
+
+ #if defined(_MSC_VER) && _MSC_VER < MSC_2015_VER
+ // strtod is broken in MSVC compilers older than 2015, so std::fixed fails
to format a double literal.
+ // more details: https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
+ //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
+ // and
+ //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
+ // note that if this function is called from the erlang generator and the
MSVC compiler is older than 2015,
+ // the double literal must be output in the scientific format. There can
be some cases where the
+ // mantissa of the output does not have fractionals, which is illegal in
Erlang.
+ // example => 10000000000000000.0 being output as 1e+16
+ double_output_stream << std::scientific <<
std::setprecision(std::numeric_limits<double>::digits10 + 1);
#else
- double_output_stream << std::fixed;
+ //question from [email protected]: Does anybody know why we avoid
scientific notation here?
+ double_output_stream << std::fixed;
+ #if 0 //maintain compatibility with previous versions of
emit_double_as_string that could generate extra trailing digits
+ const int min_precision = std::numeric_limits<double>::digits10 + 1;
+ #else //don't add unneeded 0's
+ const int min_precision = 1;
+ #endif
+ if (std::isfinite(value) && value != 0.0) {
+ double_output_stream <<
+ std::setprecision(std::max(min_precision,
+ std::numeric_limits<double>::digits10 + 1
+ -
static_cast<int>(floor(log10(abs(value))))
+ )
+ );
+ } else {
+ double_output_stream << std::setprecision(min_precision);
+ }
Review comment:
Do not indent code in `#ifdef`. Move all indenting levels in `#ifdef`
left by one please.
##########
File path: compiler/cpp/src/thrift/generate/t_erl_generator.cc
##########
@@ -389,27 +390,36 @@ void t_erl_generator::close_generator() {
const std::string emit_double_as_string(const double value) {
std::stringstream double_output_stream;
- // sets the maximum precision:
http://en.cppreference.com/w/cpp/io/manip/setprecision
- // sets the output format to fixed:
http://en.cppreference.com/w/cpp/io/manip/fixed (not in scientific notation)
- double_output_stream <<
std::setprecision(std::numeric_limits<double>::digits10 + 1);
-
- #ifdef _MSC_VER
- // strtod is broken in MSVC compilers older than 2015, so std::fixed
fails to format a double literal.
- // more details: https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
- //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
- // and
- //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
- #if _MSC_VER >= MSC_2015_VER
- double_output_stream << std::fixed;
- #else
- // note that if this function is called from the erlang generator
and the MSVC compiler is older than 2015,
- // the double literal must be output in the scientific format. There
can be some cases where the
- // mantissa of the output does not have fractionals, which is
illegal in Erlang.
- // example => 10000000000000000.0 being output as 1e+16
- double_output_stream << std::scientific;
- #endif
+
+ #if defined(_MSC_VER) && _MSC_VER < MSC_2015_VER
+ // strtod is broken in MSVC compilers older than 2015, so std::fixed fails
to format a double literal.
+ // more details: https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
+ //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
+ // and
+ //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
+ // note that if this function is called from the erlang generator and the
MSVC compiler is older than 2015,
+ // the double literal must be output in the scientific format. There can
be some cases where the
+ // mantissa of the output does not have fractionals, which is illegal in
Erlang.
+ // example => 10000000000000000.0 being output as 1e+16
+ double_output_stream << std::scientific <<
std::setprecision(std::numeric_limits<double>::digits10 + 1);
#else
- double_output_stream << std::fixed;
+ //question from [email protected]: Does anybody know why we avoid
scientific notation here?
+ double_output_stream << std::fixed;
+ #if 0 //maintain compatibility with previous versions of
emit_double_as_string that could generate extra trailing digits
+ const int min_precision = std::numeric_limits<double>::digits10 + 1;
+ #else //don't add unneeded 0's
+ const int min_precision = 1;
+ #endif
+ if (std::isfinite(value) && value != 0.0) {
+ double_output_stream <<
+ std::setprecision(std::max(min_precision,
+ std::numeric_limits<double>::digits10 + 1
Review comment:
Better have `std::numeric_limits<double>::digits10 + 1` in a const int
variable at the top of the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
