Get decimal separator from locale if available. It is not always comma. #1339

pepi124 · 2021-10-05T13:31:41Z

For example it is '/' for Persian language.

src/lib_json/json_tool.h

cdunn2001 · 2021-10-22T04:15:53Z

Anybody know what's going on with travis-ci?

Since June 15th, 2021, the building on travis-ci.org is ceased. Please use travis-ci.com from now on.

Hmmm. I'm not sure how to migrate a repo for a Github "Organization".

src/lib_json/json_tool.h

BillyDonahue · 2021-10-22T09:52:40Z

src/lib_json/json_tool.h

@@ -86,30 +86,27 @@ static inline void uintToString(LargestUInt value, char*& current) {
  } while (value != 0);
 }

-/** Change ',' to '.' everywhere in buffer.
+/** Change decimal point from current locale (i.e. ',') to '.' everywhere in buffer.


Ok I guess I don't understand the logic of the change then because this isn't what it does if your locale's decimalPoint happens to be .. In that case I'd think the function would be a noop just based on this description. But if your locale decimalPoint happens to be ., then this will replace , with ., which is a choice I don't understand.

Yes, I agree, I made this for backward compatibility (if decimal point is '.' than try to replace all ',' in buffer as it was before). From my point of view it would be better to skip replace if decimal point is '\0' or '.', I can change it this way if you are ok with it.

This is a worse breaking change than initially anticipated.
I think we should just not do it at all..

The call to getDecimalPoint() is not threadsafe.
It's a wrapper around localeconv()->decimal_point, but that's not a thread-safe function.
This PR introduces a call to it in the fixNumericLocale path.

Previously the only call was in fixNumericLocaleInput which is unused.
We should probably remove it altogether. This is a private header so removing an unsafe function shouldn't break anyone who's honoring that contract.

cdunn2001 · 2021-10-25T16:01:17Z

Please squash these into a single commit and git push -f onto your branch. That might cause TravisCI to start. Not sure. The new OSS plans are restrictive, but in theory they work on branch pushes to PRs, as of this morning.

cdunn2001 · 2021-10-28T18:43:20Z

I think we've finally solved our TravisCI migration problems. Please rebase, wait for tests to pass, and merge.

pepi124 · 2021-11-04T14:36:38Z

Rebased, tests passed. I have no rights to merge this PR.

Youw · 2023-02-24T12:03:45Z

src/lib_json/json_tool.h

@@ -19,6 +19,8 @@
 #include <clocale>
 #endif

+#include <algorithm>


this is actually a heavy dependency to include just for a single (and simple) algorithm function to use

I don't think it matters enough to optimize around it.
A header like this is likely to be brought in by other headers anyway.

never directly and not but all compilers
(and it gets better each time)

Youw · 2023-02-24T12:06:37Z

src/lib_json/json_tool.h

+  const char decimalPoint = getDecimalPoint();
+  if (decimalPoint != '\0' && decimalPoint != '.') {
+    std::replace(begin, end, decimalPoint, '.');
  }
-  return begin;
+  return end;


compared to a previous implementation it has an undesired side-effect:
if used locale-independent version of jsoncpp, the former version would replace ',' with '.' if jsons happen to have ',' as a separator
with the change - it will not happen if current locale happened to be C or something
could be non-backward-compatible side-effect

This private helper function is only ever used to correct the small buffers produced by snprintf of individual numeric values with either %.*g or %.*f format specifiers.

The idea was that snprintf is locale-dependent and JSON isn't, so we're trying to undo the possibly inappropriate decimal point. We don't have to worry about thousands separators, in particular, because we controlled the format string and didn't use a ' modifier.

To really fix this problem we would have to synchronize the getDecimalPoint() call.
We should also make sure that it doesn't change between the time that we call snprintf and the time that we try to fix up the decimal points with this function. This is all kind of unappetizing work, but this PR isn't a valid shortcut IMO. We should instead change the snprintf calls to something that doesn't depend on locale in the first place.

Because we're a library we can't control the application locale. We really need to just use a different formatter.
I think using a std::ostrstream with imbue is probably the most portable way to go.

There's also std::to_chars when the platform is C++17 or beyond.

We should instead change the snprintf calls to something that doesn't depend on locale in the first place.

Sounds briliant.

There's also std::to_chars when the platform is C++17 or beyond.

Tried that not so long ago: some compilers that mostly support C++17 might not have it, e.g. libc++ still has a partial support of it.

BillyDonahue · 2023-02-27T16:59:40Z

src/lib_json/json_tool.h

@@ -86,30 +86,27 @@ static inline void uintToString(LargestUInt value, char*& current) {
  } while (value != 0);
 }

-/** Change ',' to '.' everywhere in buffer.
+/** Change decimal point from current locale (i.e. ',') to '.' everywhere in buffer.


This is a worse breaking change than initially anticipated.
I think we should just not do it at all..

The call to getDecimalPoint() is not threadsafe.
It's a wrapper around localeconv()->decimal_point, but that's not a thread-safe function.
This PR introduces a call to it in the fixNumericLocale path.

Previously the only call was in fixNumericLocaleInput which is unused.
We should probably remove it altogether. This is a private header so removing an unsafe function shouldn't break anyone who's honoring that contract.

BillyDonahue · 2023-02-27T17:24:36Z

src/lib_json/json_tool.h

+  const char decimalPoint = getDecimalPoint();
+  if (decimalPoint != '\0' && decimalPoint != '.') {
+    std::replace(begin, end, decimalPoint, '.');
  }
-  return begin;
+  return end;


This private helper function is only ever used to correct the small buffers produced by snprintf of individual numeric values with either %.*g or %.*f format specifiers.

The idea was that snprintf is locale-dependent and JSON isn't, so we're trying to undo the possibly inappropriate decimal point. We don't have to worry about thousands separators, in particular, because we controlled the format string and didn't use a ' modifier.

To really fix this problem we would have to synchronize the getDecimalPoint() call.
We should also make sure that it doesn't change between the time that we call snprintf and the time that we try to fix up the decimal points with this function. This is all kind of unappetizing work, but this PR isn't a valid shortcut IMO. We should instead change the snprintf calls to something that doesn't depend on locale in the first place.

Because we're a library we can't control the application locale. We really need to just use a different formatter.
I think using a std::ostrstream with imbue is probably the most portable way to go.

There's also std::to_chars when the platform is C++17 or beyond.

cdunn2001 approved these changes Oct 22, 2021

View reviewed changes

src/lib_json/json_tool.h Outdated Show resolved Hide resolved

BillyDonahue reviewed Oct 22, 2021

View reviewed changes

src/lib_json/json_tool.h Outdated Show resolved Hide resolved

src/lib_json/json_tool.h Outdated Show resolved Hide resolved

BillyDonahue reviewed Oct 22, 2021

View reviewed changes

pepi124 force-pushed the fix_decimal_separator branch from 87733f9 to 3a39e27 Compare November 4, 2021 13:14

Get decimal separator from locale if available. It is not always comma.

77733c2

pepi124 force-pushed the fix_decimal_separator branch from 3a39e27 to 77733c2 Compare November 4, 2021 13:20

Fixed missing include.

d912dc3

Merge branch 'master' into fix_decimal_separator

4da3226

Youw reviewed Feb 24, 2023

View reviewed changes

BillyDonahue requested changes Feb 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get decimal separator from locale if available. It is not always comma. #1339

Get decimal separator from locale if available. It is not always comma. #1339

pepi124 commented Oct 5, 2021

cdunn2001 commented Oct 22, 2021

BillyDonahue Oct 22, 2021 •

edited

pepi124 Oct 22, 2021

BillyDonahue Feb 27, 2023

cdunn2001 commented Oct 25, 2021

cdunn2001 commented Oct 28, 2021

pepi124 commented Nov 4, 2021

Youw Feb 24, 2023

BillyDonahue Feb 27, 2023

Youw Feb 27, 2023

Youw Feb 24, 2023 •

edited by BillyDonahue

BillyDonahue Feb 27, 2023

Youw Feb 27, 2023

BillyDonahue Feb 27, 2023

BillyDonahue Feb 27, 2023

Get decimal separator from locale if available. It is not always comma. #1339

Are you sure you want to change the base?

Get decimal separator from locale if available. It is not always comma. #1339

Conversation

pepi124 commented Oct 5, 2021

cdunn2001 commented Oct 22, 2021

BillyDonahue Oct 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdunn2001 commented Oct 25, 2021

cdunn2001 commented Oct 28, 2021

pepi124 commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youw Feb 24, 2023 • edited by BillyDonahue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyDonahue Oct 22, 2021 •

edited

Youw Feb 24, 2023 •

edited by BillyDonahue