Ensures that text floats are written using '.' for decimal points regardless of locale.#366
Ensures that text floats are written using '.' for decimal points regardless of locale.#366
Conversation
e7b31db to
91d580f
Compare
nirosys
left a comment
There was a problem hiding this comment.
LGTM, might want to drop a warning about the format string though.
| if (*cp == ',') { | ||
| *cp = '.'; // Convert comma decimal separator to period | ||
| break; // No float will contain more than one decimal point |
There was a problem hiding this comment.
This will break if the provided format string specifies to provide thousands' grouping characters. For instance:
int main(int argc, char **argv) {
setlocale(LC_ALL, "it_IT.UTF-8");
printf("%'f\n", M_PI * 10000);
}Prints:
% ./test-float
31.415,926536
Since _ion_writer_vsnprintf_double is static no users would be able to actually use it, but we may want to make a note in the description that any attempt to add separators to the output could interfere with the function.
Would be awesome to get the time to visit #112 and support locales.
There was a problem hiding this comment.
Hmm... I had not considered that separators might be added elsewhere depending on locale. This isn't valid Ion, so in order to be correct we'd need to prevent that too.
A locale-independent implementation might be best. Maybe Ryu (or similar) would be suitable.
There was a problem hiding this comment.
To be clear, it's not added by the locale. It's added by the apostrophe in the format string.
I don't think a user would be able to use the function or influence the inclusion of separators with just the locale. It would just be up to us to ensure that we don't inadvertently expose the format capability to the user, and ensure we don't use the format to add separators, without changing this function.
…ardless of locale.
22b6721 to
a63bd13
Compare
Issue #, if available:
Fixes #364
Description of changes:
Previous iterations of this fix attempted to pass in a custom locale (platform-specific functions required) or temporarily modify the global locale (thread-safety issues). Instead, we use a simple solution substitutes any
,in formatted floats with..By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.