Page MenuHomeWildfire Games

Workaround differences between libc and libstd string stream parsing of float/doubles
ClosedPublic

Authored by wraitii on Jun 16 2019, 12:08 PM.

Details

Reviewers
vladislavbelov
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22908: Workaround differences between libc and libstd string stream parsing of…
Trac Tickets
#2780
Summary

Follow-up on D1978.
libc and libstd differ on how they parse strings. This has a remote chance of breaking mods or 0 A.D. code.

Add a workaround to ensure they are consistent.

Together with D1978 this fixes tests on MacOs.

Test Plan

Compile. Run tests.

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_CStr.h
|   1| /*·Copyright·(C)·2010·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2010"

source/ps/CStr.cpp
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1743/display/redirect

wraitii updated the Trac tickets for this revision.Jun 16 2019, 5:05 PM
vladislavbelov requested changes to this revision.Jun 21 2019, 9:11 PM
vladislavbelov added a subscriber: vladislavbelov.

How many lines of our code depend on this behaviour (that wrong strings are still parsed)?

Judging by the LLVM bug discussion it seems not a complete wrong behaviour, at least someone may want to parse floats like 1e-10. If the current implementation breaks some mods it means that they do something wrong, don't they?

source/ps/CStr.cpp
127 ↗(On Diff #8506)

The empty line isn't needed.

128 ↗(On Diff #8506)

I'm not sure, but there might be a way to implement the function without the string copying using iterators.

132 ↗(On Diff #8506)

We shouldn't use magic numbers here. There are std::isdigit and '...'.

141 ↗(On Diff #8506)

It's not float/double (below too). As I don't see any description about integer values.

This revision now requires changes to proceed.Jun 21 2019, 9:11 PM

How many lines of our code depend on this behaviour (that wrong strings are still parsed)?

I would certainly hope none otherwise MacOS has been behaving wrong for years.
The only place I can think of that parses strings to number is GUI sizes, which would previously parse things like "3b" as "0" instead of the 'correct' "3". I don't think there's mods or 0 A.D. code that relied on that - and I'm not sure we want that code to live.

Judging by the LLVM bug discussion it seems not a complete wrong behaviour, at least someone may want to parse floats like 1e-10.

Probably not worth the discrepancy, though. 0.0000000001 works as an alternative. Also don't believe our fixed point numbers support that precision.

If the current implementation breaks some mods it means that they do something wrong, don't they?

I would say yes.

source/ps/CStr.cpp
128 ↗(On Diff #8506)

and passing each character to stringstream individually? I doubt it's much faster, perhaps.

132 ↗(On Diff #8506)

Wasn't aware of those, thanks.

141 ↗(On Diff #8506)

?
You mean that only float/double exhibit the bug and need to be changed?
If so, I'm changing all of them for internal consistency.

I would certainly hope none otherwise MacOS has been behaving wrong for years.
The only place I can think of that parses strings to number is GUI sizes, which would previously parse things like "3b" as "0" instead of the 'correct' "3". I don't think there's mods or 0 A.D. code that relied on that - and I'm not sure we want that code to live.

Do we really need that behaviour for wrong strings?

Probably not worth the discrepancy, though. 0.0000000001 works as an alternative. Also don't believe our fixed point numbers support that precision.

That - yes, but it's easier to write 1e-3 instead of counting zeroes.

source/ps/CStr.cpp
128 ↗(On Diff #8506)

No, I mean a thing like a custom std::streambuf to prevent the string copying and something like std::istreambuf_iterator. But it requires additional measurements.

141 ↗(On Diff #8506)

Yes, only they should be changed.

For ex. . isn't a correct character for an integer parsing. Also if integer parsing works already then we don't need another pass to spend CPU time (by my measurements the function slows down the parsing on 2%-10%).

wraitii added inline comments.Jun 21 2019, 10:28 PM
source/ps/CStr.cpp
128 ↗(On Diff #8506)

Ah, I see.

I would assume that code parsing strings to floats is already slow enough that it shouldn't be hot enough for the additional code complexity to really be worth it.

Can be explored if you want.

141 ↗(On Diff #8506)

Fair enough. Will rename this then.

Do we really need that behaviour for wrong strings?

I don't think we do. Doubt we have any code in 0 A.D. that uses it, and I also doubt that any mod relies on that, because it's hardly useful anyhow.

Looks like the conversation is coming back around to where we were at D1978, but with higher confidence and better understanding.

That is, we want want to make the feature enabled cross-platform, but rather the other way around, of adjusting what input is acceptable in the test. And potentially even adding code to make the rejective behaviour that macOS offers cross-platform (or maybe just a social rule not to depend on it, with a lint or compile warning if possible).

wraitii updated this revision to Diff 8608.Jun 25 2019, 9:03 PM

Only use it for floats and doubles - clear up magic numbers.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_CStr.h
|   1| /*·Copyright·(C)·2010·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2010"

source/ps/CStr.cpp
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1816/display/redirect

source/ps/CStr.cpp
157 ↗(On Diff #8608)

What's about comment? About reason of such modification.

Also I think we make an assumption here that we accept only plain floats: not scientific nor hexadecimal.

160 ↗(On Diff #8608)

Maybe:

for (char& ch : cleaned_copy)
    if (!std::isdigit(ch) && ch != '.' && ch != '-' && ch != '+')
        ch = ' ';
wraitii added inline comments.Jun 27 2019, 8:37 AM
source/ps/CStr.cpp
157 ↗(On Diff #8608)

Will add. Is it a problem that we only accept plain floats?

160 ↗(On Diff #8608)

yeah that's better, dunno why I went old school here.

vladislavbelov added inline comments.Jun 27 2019, 9:53 AM
source/ps/CStr.cpp
157 ↗(On Diff #8608)

I mean that it may present in the comment. I don't think it's a problem in common case. The only case I see, if it's somehow received from JS (through config or something else), because JS supports scientific notation.

wraitii updated this revision to Diff 9010.Jul 20 2019, 3:07 PM

Comment + range-for

wraitii added inline comments.Jul 20 2019, 3:11 PM
source/ps/CStr.cpp
157 ↗(On Diff #8608)

Don't think it's an issue that we'll run into.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_CStr.h
|   1| /*·Copyright·(C)·2010·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2010"

source/ps/tests/test_CStr.h
|  22| class·TestCStr·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestCStr:' is invalid C code. Use --std or --language to configure the language.

source/ps/CStr.cpp
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"
Executing section JS...
Executing section cli...

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

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/181/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/182/display/redirect

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

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

Angen added a subscriber: Angen.Sep 15 2019, 1:10 PM

Tests passed

file: source\ps\CStr.cpp
line 195 : [C-style cast]: 		return (long)Pos;
line 206 : [C-style cast]: 		return (long)Pos;
line 217 : [C-style cast]: 		return (long)Pos;
line 232 : [C-style cast]: 		return (long)Pos;
line 242 : [a++ -> ++a]: 	for (size_t i = 0; i < length(); i++)
line 243 : [C-style cast]: 		NewString[i] = (tchar)_totlower((*this)[i]);
line 251 : [a++ -> ++a]: 	for (size_t i = 0; i < length(); i++)
line 252 : [C-style cast]: 		NewString[i] = (tchar)_totupper((*this)[i]);
line 349 : [a++ -> ++a]: 	for (size_t i = 0; i < length(); i++)
line 365 : [C-style cast]: 			ss << "\\u" << std::hex << std::setfill('0') << std::setw(4) << (int)(unsigned char)ch;
line 381 : [a++ -> ++a]: 			for (Left = 0; Left < length(); Left++)
line 396 : [a++ -> ++a]: 			for (Left = 0; Left < length(); Left++)
line 447 : [C-style cast]: 	return (size_t)fnv_hash(data(), length()*sizeof(value_type));
line 448 : Comments should start with capital: 		// janwas 2005-03-18: now use 32-bit version; 64 is slower and
line 461 : [a++ -> ++a]: 	for (i = 0; i < len; i++)
line 483 : [C-style cast]: 		*(str++) = (tchar)native;
line 503 : [C-style cast]: 	Serialize_int_4(buffer, (u32)len);
line 505 : [a++ -> ++a]: 	for (i = 0; i < len; i++)
file: source\ps\tests\test_CStr.h
line 82 : a+ -> a +: 		u8* buf = new u8[len+1];
line 158 : [WARNING]: File is missing include guard

}

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:

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

This revision was not accepted when it landed; it landed in state Needs Review.Sep 15 2019, 2:02 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.