Opened 9 years ago

Closed 9 years ago

#13117 closed defect (fixed)

wxConfigBase::Read changes the floating point separator after reading a double

Reported by: charras Owned by: vadz
Priority: normal Milestone:
Component: base Version: stable-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: no

Description

I am using a locale having a comma as floating point separator.
All works fine until
wxConfigBase::Read( const wxString& key, double* d, double defaultVal)
or
wxConfigBase::Read( const wxString& key, double* d)
is called.
After this call, the floating point separator go back to the defaut "C" separator (a point).
A call to setlocale(LC_NUMERIC, "") is needed to retrieve the right separator (should not).

Issue found in wxMSW 2.9.1 or wxWidgets svn version
(Note: seems a regression: works fine under 2.8)

Thanks to the wxWidgets team for all this hard work.

Attachments (1)

minimal_show_decimalsep_issue.diff download (1.1 KB) - added by charras 9 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 9 years ago by vadz

  • Status changed from new to infoneeded_new

What class are you using exactly, wxRegConfig or wxFileConfig?

If the latter, please retest with the latest svn, the code reading floating point numbers in it has been changed to use wxString::ToCDouble() which might have fixed the problem. Although to be honest I don't believe this function has ever changed the locale so it might not be related to this in any case.

If you still the problem with the latest svn, please submit a patch to the minimal sample reproducing the problem so that we could debug it. Thanks!

Changed 9 years ago by charras

comment:2 Changed 9 years ago by charras

  • Status changed from infoneeded_new to new

I am using wxRegConfig.
(I tested the latest (apr 04) svn version, and this issue still is found.
I sent you a diff file to show this issue using minimal.cpp. For me this patch shows this issue.

I expect you are using a local like French locale that uses comma as default floating point separator.
(Obviously, if the locale uses a point as separator, nothing is wrong).
I believe this is the case for you, but if I am wrong, I'll try to send a better patch.

Thanks.

comment:3 Changed 9 years ago by vadz

  • Status changed from new to infoneeded_new

I don't understand what's going on because this works for me, here is the message box I see (result of pressing Ctrl-C in it and pasting here):

[Window Title]
before read 1,000000

[Content]
After read 1,000000

[ОК]

So the bug seems to be specific to your system and I really don't know what to do about it. My only idea is to try to find where exactly is the locale being changed by adding calls to wxLogDebug("%f", 1.234) here and there...

comment:4 Changed 9 years ago by charras

  • Status changed from infoneeded_new to new

I did it, using Mingw, gcc 4.4.0, and wxWidgets 2.9 (april 04) svn version.
Here is what I found:
the culprit is (for me) in xlocale.cpp.
The macro IMPLEMENT_STRTOX_L_END does restore the "old" locale,
because macro IMPLEMENT_STRTOX_L_START does not save the "old" locale.

The line
const char *oldLocale = wxSetlocale(LC_NUMERIC, "C");
saves the new locale setup, not the old
(in fact oldLocale string contains "C" and stores the new locale and not the old))

As a proof, I added 2 lines to save the old locale in IMPLEMENT_STRTOX_L_START macro:
char prvlocale[255]; /* buffer to store the current locale name */
strcpy( prvlocale, wxSetlocale(LC_NUMERIC, NULL) ); /* get and store the current locale */

and a minor change in IMPLEMENT_STRTOX_L_END
wxSetlocale(LC_NUMERIC, prvlocale); instead of wxSetlocale(LC_NUMERIC, oldLocale );

Remember it is just an ugly test code (I usually write better code).

Because thes 2 macros are conditionnaly compiled (#ifndef wxHAS_XLOCALE_SUPPORT in xlocale.cpp),
this issue is depending on compiler version.

Here is my test code (that fixes my issue):
#define IMPLEMENT_STRTOX_L_START \

wxCHECK(loc.IsOk(), 0); \

\

/* (Try to) temporary set the 'C' locale */ \
char prvlocale[255]; \
strcpy( prvlocale, wxSetlocale(LC_NUMERIC, NULL) ); \
const char *oldLocale = wxSetlocale(LC_NUMERIC, "C"); \
if ( !oldLocale ) \
{ \

/* the current locale was not changed; no need to */ \
/* restore the previous one... */ \
errno = EINVAL; \

/* signal an error (better than nothing) */ \

return 0; \

}

#define IMPLEMENT_STRTOX_L_END \

/* restore the original locale */ \
wxSetlocale(LC_NUMERIC, prvlocale); \
return ret;


comment:5 Changed 9 years ago by vadz

  • Owner set to vadz
  • Status changed from new to accepted

Thanks for investigating this, the current code is indeed clearly incorrect, I don't really understand how did it work for me to be honest (but it definitely did, albeit with MSVC and not MinGW). I'll fix this in the way you proposed.

I also think that we probably should avoid using this code in the first place when we don't have real xlocale support (i.e. wxHAS_XLOCALE_SUPPORT is not defined) as changing the locale of all threads globally just to format a number using decimal point is really not a good idea (for recent versions of MSVC this could be avoid by using _configthreadlocale() but this is not portable).

comment:6 Changed 9 years ago by vadz

P.S. The reason for which I didn't see the bug is, of course, that wxHAS_LOCALE_SUPPORT is defined for MSVC9 that I use so the wrong code wasn't used at all in my build, sorry for not realizing this sooner.

comment:7 Changed 9 years ago by VZ

(In [67405]) Fix incorrect use of setlocale() in wxLocale::IsAvailable().

The return value of setlocale() was used incorrectly in this code: it
represents the newly set locale and not the previously active one so we didn't
actually restore the original locale before.

Fix the code and check that we do actually restore the locale in a new unit
test for it.

See #13117.

comment:8 Changed 9 years ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [67406]) Correctly restore the old locale in wxXLocale functions.

In non-wxHAS_XLOCALE_SUPPORT case we didn't restore the original locale
correctly in wxStrtoxxx_l() functions as the return value of wxSetlocale() was
incorrectly assumed to be the old locale instead of the new one.

Fix this and also replace the macros used by the old code with a small helper
class, this simplifies the code and is less ugly.

Finally add a unit test which failed before these changes when the program ran
in any non-C locale but passes now.

Closes #13117.

Note: See TracTickets for help on using tickets.