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
Compile. Run tests.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- temp
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 7966 Build 12966: Vulcan Build Jenkins Build 12965: arc lint + arc unit
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
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 | The empty line isn't needed. | |
128 | I'm not sure, but there might be a way to implement the function without the string copying using iterators. | |
132 | We shouldn't use magic numbers here. There are std::isdigit and '...'. | |
141 | It's not float/double (below too). As I don't see any description about integer values. |
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 | and passing each character to stringstream individually? I doubt it's much faster, perhaps. | |
132 | Wasn't aware of those, thanks. | |
141 | ? |
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 | 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 | 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%). |
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).
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 | ||
---|---|---|
169 | 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. |
source/ps/CStr.cpp | ||
---|---|---|
169 | 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
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