Page MenuHomeWildfire Games

Fix incorrect use of setlocale() in cppformat CStr tests
ClosedPublic

Authored by wraitii on Jun 26 2019, 1:50 AM.

Details

Summary

Follows-up rP22378.

Test Plan

Passing Jenkins build.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8080
Build 13151: Vulcan BuildJenkins
Build 13150: arc lint + arc unit

Event Timeline

Krinkle created this revision.Jun 26 2019, 1:50 AM

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

elexis edited reviewers, added: wraitii; removed: elexis, Restricted Owners Package.Jun 26 2019, 3:20 AM
source/ps/tests/test_CStr.h
1

9 (can be changed when committing)

source/ps/tests/test_cppformat.h
1

9

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 -_-

Agreed, I think we should also protect these calls behind a global mutex, so that it actually becomes threadsafe.

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):

  1. locale=default
  2. T1::ScopedLocale::ctor(L1), locale=L1
  3. T2::ScopedLocale::ctor(L2), locale=L2
  4. T1::ScopedLocale::dtor(), locale=default
  5. T2::ScopedLocale::dtor(), locale=L1

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):

Indeed, which is why I would lock this behind a global mutex, so it goes

  1. locale=default
  2. T1::ScopedLocale::ctor(L1), locale=L1
  3. T2::ScopedLocale::ctor(L2) - blocked
  4. T1::ScopedLocale::dtor(), locale=default
  5. resuming of T2::ScopedLocale::ctor(L2) - locale=L2
  6. T2::ScopedLocale::dtor(), locale=default

Indeed, which is why I would lock this behind a global mutex, so it goes

  1. locale=default
  2. T1::ScopedLocale::ctor(L1), locale=L1
  3. T2::ScopedLocale::ctor(L2) - blocked
  4. T1::ScopedLocale::dtor(), locale=default
  5. resuming of T2::ScopedLocale::ctor(L2) - locale=L2
  6. 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.

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.

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.

@Krinkle Feel up for implementing ScopedLocale? If not, I can commandeer this.

wraitii commandeered this revision.Jul 1 2019, 9:27 PM
wraitii updated this revision to Diff 8683.
wraitii edited reviewers, added: Krinkle; removed: wraitii.

Use ScopedLocale.

I've put it in lib for lack of a better place.

Vulcan added a comment.Jul 1 2019, 9:50 PM

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

Stan added a subscriber: Stan.Jul 1 2019, 10:42 PM
Stan added inline comments.
source/lib/scoped_locale.h
31 ↗(On Diff #8683)

nullptr.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2019, 2:57 PM
This revision was automatically updated to reflect the committed changes.
elexis added a subscriber: elexis.Jul 20 2019, 3:14 PM
/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");