locale aware double->string conversions #467
Conversation
the standard xlnt::cell and xlnt::cell_reference have plenty of extra functionality that just slows things down during (de)serialisation These intermediate structs can be used to minimise overhead before transforming to the final type
…ach possible row/column combination Not tested, definitely not as correct as previous implementation
…up for each possible row/column combination" This reverts commit 63484f8.
…ormed strod / stod / to_string and all related friends are dependant on current locale for how they format a number
| } | ||
|
|
||
| double deserialise(const std::string &s) const | ||
| double deserialise(const std::string &s, ptrdiff_t *len_converted) const |
There was a problem hiding this comment.
Change here allows deserialise to be used anywhere std::strtod was used. Matching the char** param is however not possible due to the temporary buffer used when locale conversion is required. In the one case this was required for, the number of characters converted was what was being checked anyway.
| @@ -0,0 +1,96 @@ | |||
| #ifndef XLNT_DETAIL_SERIALISATION_HELPERS_HPP | |||
| #define XLNT_DETAIL_SERIALISATION_HELPERS_HPP | |||
There was a problem hiding this comment.
this was moved out of xlsx_consumer.cpp as a bit of cleanup from #421
I was orginally thinking about making similar changes to the serialiser but decided against it. This remains as a separate file as it probably always should have been (in general, consumers is too large already)
| // std::string attribute name | ||
| // not integer or float type | ||
| template <typename T, typename = typename std::enable_if<!std::is_convertible<T, double>::value>::type> | ||
| void write_attribute(const std::string &name, T value) | ||
| { | ||
| current_part_serializer_->attribute(name, value); | ||
| } | ||
|
|
||
| template<typename T> | ||
| void write_attribute(const std::string &name, double value) | ||
| { | ||
| current_part_serializer_->attribute(name, converter_.serialise(value)); | ||
| } | ||
|
|
||
| // qname attribute name | ||
| // not integer or float type | ||
| template <typename T, typename = typename std::enable_if<!std::is_convertible<T, double>::value>::type> | ||
| void write_attribute(const xml::qname &name, T value) | ||
| { | ||
| current_part_serializer_->attribute(name, value); | ||
| } | ||
|
|
||
| template<typename T> | ||
| void write_attribute(const xml::qname &name, double value) | ||
| { | ||
| current_part_serializer_->attribute(name, converter_.serialise(value)); | ||
| } |
There was a problem hiding this comment.
the little dance going on here is redirecting all numbers (to be precise, types convertible to double) through the untemplated functions with double as the parameter. Without the enable if on the general case, the overload would only catch exact matches (i.e. double but not float/int).
Unlike recent PR's from me, this is almost entirely a correctness improvement.
With this branch, local runs of the test suite have no failures even when the locale is changed to one which uses the comma character as its decimal separator (i.e. passes with set_locale(LC_ALL, "de-DE") on windows. the german locale name may be different on other platforms)
This is done by removing all usages of the following functions when applied to floating point types (e.g. double) to convert them to a string representation
Instead these number_serialiser::serialise and number_serialiser::serialise_short use the same technique of coverting the comma to a decimal point when required as was used in #421 to affect the reverse transformation (string->double)
I'm not sure of the best way to add a locale check to CI. Duplicating one of the round trip tests (all formats is probably the best one for this) and modifying the locale while it runs would work, but there's still the issue of different names on different platforms, and which (if any) alternative locales are installed by default
locale tests on windows can be affected by adding the following to the start of the test main
setlocale(LC_ALL, "de-DE");default locale can be restored by putting "C" as the locale name