Page MenuHomePhabricator

Set locales only for the scope of the current reader/writer
Closed, ResolvedPublic

Description

In some Readers/Writers the locales are manipulated in a global scope. This should not be done. Every reader/writer should set it's locales in a local scope and reset the locales to the previous system locales after the read/write operations are finished to prevent the program from strange behaviour.

Event Timeline

I think most of these issues existed already before the new I/O sub-system.

For the mitk::ItkImageIO I created a "scoped locale switcher" which resets the locale to the original value when it goes out of scope (function return or exception). Maybe we can extract that class and export it from the Core module such that it can be reused in different reader/writer implementations:

https://github.com/MITK/MITK/blob/master/Core/Code/Internal/mitkItkImageIO.cpp#L339

To extract the functionality would be nice. At the moment the following classes are affected with this problem:

Core/Code/Internal/mitkPointSetReaderService.cpp
Modules/ContourModel/IO/mitkContourModelReader.cpp
Modules/ContourModel/IO/mitkContourModelSetReader.cpp
Modules/LegacyIO/mitkPointSetReader.cpp

Already got it and running the tests. At the moment there is no generic functionality for all readers, e.g. the same 3 lines of code are copy&pasted in every reader.

I looked into the class hierarchy. The classes are inherited from different superclasses, mitk::IFileReader/mitk::AbstractFileReader or mitk::FileReader. To extract the locale functionality it would be nice if all readers are implementing the same interface.

User heime has pushed new remote branch:

bug-18418-setLocaleIO

Well, this problem stems from supporting legacy code / being backwards compatible.

Andreas and I discussed creating a wrapper about IFileReader which will add default properties to all base data objects. This could also be used to switch to the C locale for all readers / writers automatically.

We would definitely need the LocaleSwitch class. Fixing the buggy readers / writers that way is fine. Replacing it with a general system can be done later.

Nice. Do you have a branch for code review?

Found it (if bug-18418-setLocaleIO contains the latest changes). I am not totally happy with the proposed changes: We should take that bug as an opportunity the use the descriped helper class (the "scoped locale switcher"). It could be used in mitk::IOUtil::Load/Save and in the reader/writer implementations themselve.

The current code changes contain at least one code path where the locale may not be reset to the original value (due to an exception).

Didn't saw the scope class. We should move it to a seperate header and use it in the other readers.

That was exactly the content of comment #1.

User webechr has pushed new remote branch:

bug-18418-FixLocales

Please don't put implementation details in header files. Create a mitkLocaleSwitch.cpp file and move the definitions.

[1843a1]: Merge branch 'bug-18418-FixLocales'

Merged commits:

2014-12-03 14:40:01 Christian Weber [76320c]
Fix locale switch