Page MenuHomeWildfire Games

Re-D2013 - Fix incorrect use of setlocale() in cppformat CStr tests
Needs RevisionPublic

Authored by wraitii on Jul 20 2019, 3:26 PM.

Details

Reviewers
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

rP22518 failed somehow. Try again as a diff.

Based on a diff by @vladislavbelov

Test Plan

Run CI.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
temp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8505
Build 13909: Vulcan BuildJenkins
Build 13908: arc lint + arc unit

Event Timeline

wraitii created this revision.Jul 20 2019, 3:26 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/145/display/redirect

elexis added a subscriber: elexis.Jul 20 2019, 3:30 PM

(Missing new file and credit)

source/ps/tests/test_cppformat.h
1

9

wraitii updated this revision to Diff 9013.Jul 20 2019, 3:33 PM
wraitii edited the summary of this revision. (Show Details)
wraitii added a subscriber: vladislavbelov.

Add missing file.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/146/display/redirect

wraitii updated this revision to Diff 9038.Jul 21 2019, 5:24 PM

This might compile, probs still warning.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/170/display/redirect

historic_bruno added inline comments.
source/ps/tests/test_CStr.h
116

Why the change from fr_FR.UTF-8 to en_US here? That seems to be the reason for the test (see comment, and rP8218)

historic_bruno added inline comments.Jul 21 2019, 5:43 PM
source/lib/scoped_locale.h
31

Indenting issue here

wraitii added a subscriber: Itms.Jul 21 2019, 5:56 PM

Well it ought to be a common locale, and it makes as much sense as fr_FR given the numeric parsing we do.

However jenkins still doesn't appear to have it, so I'll wait until @Itms returns.

historic_bruno added a comment.EditedJul 21 2019, 6:12 PM

Well it ought to be a common locale, and it makes as much sense as fr_FR given the numeric parsing we do.

fr_FR is useful here because "," is the radix point instead of "." test_parse is intended to verify that changing the locale won't break parsing, should our implementation change.

The only thing it wouldn't catch is if the user's locale is already fr_FR, so I would support adding two blocks or two test functions, one for en_US.UTF-8 and one for fr_FR.UTF-8, specifically to handle both cases.

vladislavbelov added inline comments.Jul 29 2019, 2:24 PM
source/lib/scoped_locale.h
32

May use TS_ASSERT_DIFFERS(setlocale(m_Category, newLocale), nullptr) for a more detailed log.

43

Without #ifndef.

source/ps/tests/test_CStr.h
19

Not alphabetic order.

Stan requested changes to this revision.May 15 2020, 6:22 PM
Stan added a subscriber: Stan.

Needs to be rebased after rP23562 see comments there for what I believe to be the same bug.

This revision now requires changes to proceed.May 15 2020, 6:22 PM