Follows-up rP22378.
Details
Passing Jenkins build.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/ps/tests/test_cppformat.h | 1| /*·Copyright·(C)·2015·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2015" source/ps/tests/test_CStr.h | 1| /*·Copyright·(C)·2010·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2010" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1819/display/redirect
I think the following helper could be helpful, at least it may prevent such bugs in reusing:
class ScopedLocale { public: ScopedLocale(int category, char* newLocale) : m_Category(category), m_OldLocale(setlocale(category, NULL)) { TS_ASSERT(setlocale(m_Category, newLocale) != nullptr); } ~ScopedLocale() { setlocale(m_Category, m_OldLocale); } private: int m_Category; char* m_OldLocale; }; // Usage void test_parse() { ScopedLocale scopedLocale(LC_NUMERIC, "fr_FR.UTF-8"); // ... }
Also I think it makes sense to check that the setlocale call was successful.
Agreed, I think we should also protect these calls behind a global mutex, so that it actually becomes threadsafe.
The fact that setlocale with a null argument returns the current locale, and otherwise it returns the new locale is such a s*** API tbh -_-
If you'd use the current implementation of ScopedLocale in multiple threads, then it highly likely wouldn't work. For ex (T1 - thread N1, T2 - thread N2, L1 - locale N1, L2 - locale N2):
- locale=default
- T1::ScopedLocale::ctor(L1), locale=L1
- T2::ScopedLocale::ctor(L2), locale=L2
- T1::ScopedLocale::dtor(), locale=default
- T2::ScopedLocale::dtor(), locale=L1
Indeed, which is why I would lock this behind a global mutex, so it goes
- locale=default
- T1::ScopedLocale::ctor(L1), locale=L1
- T2::ScopedLocale::ctor(L2) - blocked
- T1::ScopedLocale::dtor(), locale=default
- resuming of T2::ScopedLocale::ctor(L2) - locale=L2
- T2::ScopedLocale::dtor(), locale=default
Yeah. By the way where it can be useful? Do we have such cases?
Implementation for this could look like:
class ScopedLocale { public: ScopedLocale(int category, char* newLocale) : m_Category(category) { static std::mutex mutex; m_LockGuard = std::lock_guard<std::mutex>(mutex); m_OldLocale = setlocale(category, NULL); TS_ASSERT(setlocale(m_Category, newLocale) != nullptr); } ~ScopedLocale() { setlocale(m_Category, m_OldLocale); } private: int m_Category; char* m_OldLocale; std::lock_guard<std::mutex> m_LockGuard; };
I don't think we have a use for it right now, no. So it could be seen as being too much future-proofing.
Will it be used sometime? Currently I don't see a reason to run multiple threads (not processes) with own locales waiting each other.
I guess the question is "will we ever run different threads with different locales". Impossible to see, the future is. But this seems like something weird to be doing.
So perhaps let's not bother with the mutex.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/ps/tests/test_cppformat.h | 1| /*·Copyright·(C)·2015·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2015" source/ps/tests/test_CStr.h | 1| /*·Copyright·(C)·2010·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2010" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1871/display/redirect
source/lib/scoped_locale.h | ||
---|---|---|
31 ↗ | (On Diff #8683) | nullptr. |
/path/to/trunk/source/ps/tests/test_CStr.h: In member function ‘void TestCStr::test_parse()’: /path/to/trunk/source/ps/tests/test_CStr.h:117:28: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings] 117 | ScopedLocale(LC_NUMERIC, "en_US.UTF-8"); | ^~~~~~~~~~~~~ test_cppformat.cpp In file included from ../../../source/ps/tests/test_cppformat.cpp:17: /path/to/trunk/source/ps/tests/test_cppformat.h: In member function ‘void TestCppformat::test_basic()’: /path/to/trunk/source/ps/tests/test_cppformat.h:29:24: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings] 29 | ScopedLocale(LC_ALL, "en_US.UTF-8");