The POSIX ecvt() function has been meked as 'LEGACY' and is not included in FreeBSD libc. Therefore FCollada, as it stands, cannot be compiled in FreeBSD or any other system without ecvt(). This patch gets around this by defining a function ecvt_musl() that is taken from the function **ecvt()** in musl libc (MIT licensed). This implementation of ecvt() only depends on sprintf() and atoi() both of which are part of the POSIX specification.
Details
- Reviewers
Itms - Commits
- rP24323: Fix FCollada on platforms without ecvt
- Trac Tickets
- #1325
I have successfully built FCollada on FreeBSD using this patch (I have not however, managed to build 0ad on FreeBSD yet, as I am having other issues in the gmake stage).
I have successfully build the whole 0ad under Fedora Linux with this patch and it runs well, as far as i can tell. However, I have not specifically tested Collada functionality, so someone with sufficient knowledge needs to test for that.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 14118 Build 29506: arc lint + arc unit
Event Timeline
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
25 | Update the copyright header. I am not sure this would be enough. What about snprintf? |
Hi @smiley
Update the copyright header. I am not sure this would be enough.
What other info should be added? Something like "License of musl libc - MIT license"?
What about snprintf?
What advantages would this bring in relation to the current musl libc implementation? If there are actual advantages, we could consider using it.
Or if this could be less cstyle, stringstream?
If going for c++11, to_string?
Is C++11 required for new code submissions? I am a terrible C++ programmer, so I cannot help here.
As I have mentioned in the test plan, I have not tested Collada functionality with the new code. That needs to be done by someone knowledgeable.
What other info should be added? Something like "License of musl libc - MIT license"?
What advantages would this bring in relation to the current musl libc implementation? If there are actual advantages, we could consider using it.
ecvt is legacy and the new alternative were the snprintf and related functions.
Is C++11 required for new code submissions? I am a terrible C++ programmer, so I cannot help here.
First of all, I am reviewing this in hopes of it being a better alternative to the fix in a certain other fork of 0ad (search the forums if interested), and I would prefer to move the codebase I commit to a later version of the standard. As for the conventions here, I don't know. None of them offered any reviews here yet, so maybe they are okay with it. In the coming weekend, I might try updating this to at least C++11, and probably upload that specific patch here.
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
22 | Given that there is no upstream of FCollada anymore, the burden of maintaining this falls on the projects using this. So, even if not related, let the records reflect this. While at it, nuke the NULL check. (https://isocpp.org/wiki/faq/freestore-mgmt#delete-handles-null) |
Re: testing;
It does work as advertised. I just think the fix could be better.
Thank you for your work so far.
-Updated copyright notices;
-'Nuked' the NULL check (smiley gives a link for isocpp where it reads that the C++ language guarantees that delete p will do nothing if p is null. Apparently, the behaviour of free() in C is the same, https://pubs.opengroup.org/onlinepubs/009695399/functions/free.html);
-Interestingly enough, pointer tmp was not being dereferenced in the for loop in ecvt_musl() (must have been a copy/past typo when copying from musl libc?). This has been fixed (now the loop is for [j=0; tmp[i]!='e'; j+=(tmp[i++]!='.')) ]. Sometimes when i log out i still see the previous version of this loop before the fix, not entirely sure why.
-Replaced spaces by tabs.
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
27 | Usually those are for a good reason. I'll try that tomorrow. @smiley did you members of the other fork fix fcollada segfaulting on vs2015+ ? |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
27 | Not sure about vs2015, I didn't build with that. And honestly, I am not that well versed on Windows and VS quirks. It does work with vs2019 with win 10 toolsets for sure. That much I can confirm. Kinda late reply. But yeah. |
Rebase. I've made some header fixes so that it doesn't affect Windows and other non BSD oses.
There is a flaw with the implementation however, as it doesn't handle NAN QNAN FLOAT_MAX/INFINITY and FLOAT_MIN (It will segfault) it seems however that
- We never have those values in Collada
- Apple's implementation has the same flaws https://opensource.apple.com/source/Libc/Libc-262/ppc/gen/ecvt.c.auto.html
- ReactOS does something different https://doxygen.reactos.org/db/d76/ecvt_8c_source.html
- I wrote a C++ alternative, but the code is not cleaner, so I think I will go with the current uploaded patch;
- Windows does not have the flaw.
char* ecvt(double x, int n, int *dp, int *sign) { char tmp[32]; n = std::min(n, 15); std::sprintf(tmp, "%.*e", n - 1, x); std::string str(tmp); *sign = str[0] == '-'; if (str == "inf" || str == "-inf") { *dp = 1; return "1#INF"; } else if ((str == "nan" || str == "-nan")) { *dp = 1; return "1#QNAN"; } else if ((str == "-nan(ind)" || str == "-nan(ind)")) { *dp = 1; return "1#IND"; } else { *dp = std::atoi(tmp + str.find('e') + 1) + 1; } int index = 0; str.erase( std::remove_if( str.begin(), str.end(), [&index, n](unsigned char x){ bool ShouldRemove = index > n || x == '+' || x == 'e' || x == '-' || x == '.'; if(!ShouldRemove) ++index; return ShouldRemove; } )); char *cstr = new char[str.length() + 1]; strcpy(cstr, str.c_str()); return cstr; }
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | Why exactly is this not std::to_string()? |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | Because std::to_string gives different results? -----------------------3.14159-------------------------- New implem++: -3.14159 Native implem: -3.14159 SNPRINTF implem: -3.141593 to_string: -3.141593 -----------------------3.14159-------------------------- New implem++: -3.14159 Native implem: -3.14159 SNPRINTF implem: -3.141593 to_string: -3.141593 -----------------------12415.1-------------------------- New implem++: -12415.1 Native implem: -12415.1 SNPRINTF implem: -12415.141602 to_string: -12415.141602 ----------------------12415.1-------------------------- New implem++: 12415.1 Native implem: 12415.1 SNPRINTF implem: 12415.141602 to_string: 12415.141602 ----------------------12415.1-------------------------- New implem++: 12415.1 Native implem: 12415.1 SNPRINTF implem: 12415.120117 to_string: 12415.120117 ----------------------12415.1-------------------------- New implem++: 12415.1 Native implem: 12415.1 SNPRINTF implem: 12415.120117 to_string: 12415.120117 ----------------------0.120341-------------------------- New implem++: 0.120341 Native implem: 0.120341 SNPRINTF implem: 0.120341 to_string: 0.120341 -----------------------0.120341-------------------------- New implem++: -0.120341 Native implem: -0.120341 SNPRINTF implem: -0.120341 to_string: -0.120341 ----------------------0.5-------------------------- New implem++: 0.5 Native implem: 0.5 SNPRINTF implem: 0.500000 to_string: 0.500000 ----------------------0-------------------------- New implem++: 0 Native implem: 0 SNPRINTF implem: 0.000000 to_string: 0.000000 ----------------------400000-------------------------- New implem++: 400000 Native implem: 400000 SNPRINTF implem: 400000.000000 to_string: 400000.000000 ----------------------4e-05-------------------------- New implem++: 0.00004 Native implem: 0.00004 SNPRINTF implem: 0.000040 to_string: 0.000040 -----------------------4e-05-------------------------- New implem++: -0.00004 Native implem: -0.00004 SNPRINTF implem: -0.000040 to_string: -0.000040 ----------------------400000-------------------------- New implem++: 400000 Native implem: 400000 SNPRINTF implem: 400000.000000 to_string: 400000.000000 ----------------------4e+25-------------------------- New implem++: 4e25 Native implem: 4e25 SNPRINTF implem: 39999998248094104989728768.000000 to_string: 39999998248094104989728768.000000 -----------------------4e+25-------------------------- New implem++: -4e25 Native implem: -4e25 SNPRINTF implem: -39999998248094104989728768.000000 to_string: -39999998248094104989728768.000000 ----------------------4e-25-------------------------- New implem++: 0 Native implem: 0 SNPRINTF implem: 0.000000 to_string: 0.000000 -----------------------4e-25-------------------------- New implem++: -0 Native implem: -0 SNPRINTF implem: -0.000000 to_string: -0.000000 ----------------------3.40282e+38-------------------------- New implem++: 3.40282e38 Native implem: 3.40282e38 SNPRINTF implem: 340282346638528859811704183484516925440.000000 to_string: 340282346638528859811704183484516925440.000000 ----------------------1.17549e-38-------------------------- New implem++: 0 Native implem: 0 SNPRINTF implem: 0.000000 to_string: 0.000000 ----------------------inf-------------------------- New implem++: 1.#INF Native implem: 1.#INF SNPRINTF implem: inf to_string: inf ----------------------0-------------------------- New implem++: 0 Native implem: 0 SNPRINTF implem: 0.000000 to_string: 0.000000 -----------------------3.40282e+38-------------------------- New implem++: -3.40282e38 Native implem: -3.40282e38 SNPRINTF implem: -340282346638528859811704183484516925440.000000 to_string: -340282346638528859811704183484516925440.000000 -----------------------1.17549e-38-------------------------- New implem++: -0 Native implem: -0 SNPRINTF implem: -0.000000 to_string: -0.000000 -----------------------inf-------------------------- New implem++: -1.#INF Native implem: -1.#INF SNPRINTF implem: -inf to_string: -inf -----------------------0-------------------------- New implem++: -0 Native implem: -0 SNPRINTF implem: -0.000000 to_string: -0.000000 ----------------------nan-------------------------- New implem++: 1.#QNAN Native implem: 1.#QNAN SNPRINTF implem: nan to_string: nan -----------------------nan(ind)-------------------------- New implem++: -1.#IND Native implem: -1.#IND SNPRINTF implem: -nan(ind) to_string: -nan(ind) ----------------------nan-------------------------- New implem++: 1.#QNAN Native implem: 1.#QNAN SNPRINTF implem: nan to_string: nan -----------------------nan-------------------------- New implem++: -1.#QNAN Native implem: -1.#QNAN SNPRINTF implem: -nan to_string: -nan ----------------------nan-------------------------- New implem++: 1.#QNAN Native implem: 1.#QNAN SNPRINTF implem: nan to_string: nan -----------------------nan(ind)-------------------------- New implem++: -1.#IND Native implem: -1.#IND SNPRINTF implem: -nan(ind) to_string: -nan(ind) ----------------------nan-------------------------- New implem++: 1.#QNAN Native implem: 1.#QNAN SNPRINTF implem: nan to_string: nan -----------------------nan(ind)-------------------------- New implem++: -1.#IND Native implem: -1.#IND SNPRINTF implem: -nan(ind) to_string: -nan(ind) ----------------------1-------------------------- New implem++: 1 Native implem: 1 SNPRINTF implem: 1.000000 to_string: 1.000000 ----------------------inf-------------------------- New implem++: 1.#INF Native implem: 1.#INF SNPRINTF implem: inf to_string: inf ----------------------inf-------------------------- New implem++: 1.#INF Native implem: 1.#INF SNPRINTF implem: inf to_string: inf -----------------------inf-------------------------- New implem++: -1.#INF Native implem: -1.#INF SNPRINTF implem: -inf to_string: -inf -----------------------inf-------------------------- New implem++: -1.#INF Native implem: -1.#INF SNPRINTF implem: -inf to_string: -inf |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | I suppose to_string has 6 digit precision. However, doesn't seem like that would be an issue to be honest. After all, and I am not even sure if the precision beyond that actually has an impact on the model. Seems like a lot of work for one insignificant digit. If needed, you can always use std::ostringstream str; str.precision(n_digits); out << std::fixed << numeric_value; |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | void FloatToString(double f, char* sz) { std::ostringstream str; str.precision(6); str << std::fixed << f; std::string out = str.str(); char *cstr = new char[out.length() + 1]; strcpy(cstr, out.c_str()); sz = cstr; } Still doesn't solve the exponential display for big numbers. eg. New implem++: -3.40282e38 Native implem: -3.40282e38 SNPRINTF implem: -340282346638528859811704183484516925440.000000 to_string: -340282346638528859811704183484516925440.000000 Does reduce the function below drastically though :D |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | Ah, that is a case I hadn't considered. It's either to_string with 6 digit precision or using this which uses fixed which cannot represent the upper limits. Even after handling the edge cases, honestly either of these variants are better than arguably putting very C like code in this. FCollada has no resemblance to modern C++, but better to not actively make it worse. |
Maybe then
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | The last issue remaining is that it breaks all the tests because of stuff like ----------------------400000-------------------------- New implem++: 400000 Native implem: 400000 SNPRINTF implem: 400000.000000 to_string: 400000.000000 OSSTRINGSTREAM: 400000.000000 or -----------------------4e-05-------------------------- New implem++: -0.00004 Native implem: -0.00004 SNPRINTF implem: -0.000040 to_string: -0.000040 OSSTRINGSTREAM: -0.000040 Admittedly, we don't care much about the tests, but since I just fixed them, I guess having something that works without having to change all the files would be nice... |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | FCollada tests or the pyro ones? If the latter, they can't be left broken on master. |
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp | ||
---|---|---|
31 | FCollada tests. |
It seems the NaN QNaN and Infinity cases are handled by the function that calls FloatToString
so the code could be
char* ecvt(double x, int n, int *dp, int *sign) { char tmp[32]; n = std::min(n, 15); std::sprintf(tmp, "%.*e", n - 1, x); std::string str(tmp); *sign = str[0] == '-'; *dp = std::atoi(tmp + str.find('e') + 1) + 1; int index = 0; str.erase( std::remove_if( str.begin(), str.end(), [&index, n](unsigned char x){ bool ShouldRemove = index > n || x == '+' || x == 'e' || x == '-' || x == '.'; if(!ShouldRemove) ++index; return ShouldRemove; } )); char *cstr = new char[str.length() + 1]; strcpy(cstr, str.c_str()); return cstr; }