Page MenuHomeWildfire Games

[BSD] Replace ecvt() in FCollada with something else
ClosedPublic

Authored by NF on Nov 1 2019, 5:00 PM.

Details

Summary

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.

Test Plan

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

NF created this revision.Nov 1 2019, 5:00 PM
lyv added a subscriber: lyv.Nov 2 2019, 11:55 AM
lyv added inline comments.
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp
25

Update the copyright header. I am not sure this would be enough.

What about snprintf?
Or if this could be less cstyle, stringstream?
If going for c++11, to_string?

NF added a comment.Nov 14 2019, 5:31 PM

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.

lyv added a comment.Nov 14 2019, 6:46 PM

What other info should be added? Something like "License of musl libc - MIT license"?

https://code.wildfiregames.com/source/0ad/browse/ps/trunk/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp$1-13

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.
The standard specifies that delete should do the check and delete NULL is no op.

(https://isocpp.org/wiki/faq/freestore-mgmt#delete-handles-null)

lyv added a comment.EditedNov 14 2019, 7:06 PM

Re: testing;

It does work as advertised. I just think the fix could be better.

Thank you for your work so far.

NF updated this revision to Diff 10374.EditedNov 21 2019, 8:50 PM

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

Stan added a subscriber: Stan.Nov 21 2019, 9:07 PM
Stan added inline comments.
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+ ?

Stan removed 1 blocking reviewer(s): Itms.Aug 1 2020, 4:41 PM
lyv added inline comments.Aug 1 2020, 4:47 PM
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.

Stan updated this revision to Diff 14294.EditedNov 30 2020, 1:22 PM

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

  1. We never have those values in Collada
  2. Apple's implementation has the same flaws https://opensource.apple.com/source/Libc/Libc-262/ppc/gen/ecvt.c.auto.html
  3. ReactOS does something different https://doxygen.reactos.org/db/d76/ecvt_8c_source.html
  4. I wrote a C++ alternative, but the code is not cleaner, so I think I will go with the current uploaded patch;
  5. 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;
}
Herald added 1 blocking reviewer(s): Itms. · View Herald TranscriptNov 30 2020, 1:22 PM
Stan retitled this revision from #1325 Replace ecvt() in FCollada with something else to [BSD] Replace ecvt() in FCollada with something else.Nov 30 2020, 1:31 PM
Stan removed a reviewer: Itms.
lyv added inline comments.Nov 30 2020, 4:22 PM
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp
31

Why exactly is this not std::to_string()?

Stan added a comment.EditedNov 30 2020, 4:33 PM
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
lyv added inline comments.Nov 30 2020, 4:58 PM
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;
Stan added inline comments.Nov 30 2020, 5:53 PM
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

lyv added inline comments.Nov 30 2020, 7:16 PM
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.

Stan added a comment.Nov 30 2020, 7:35 PM

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

lyv added inline comments.Dec 1 2020, 8:24 AM
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.

Stan added inline comments.Dec 1 2020, 8:40 AM
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp
31

FCollada tests.

Stan added a comment.Dec 1 2020, 9:24 AM

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;
}
Stan updated this revision to Diff 14368.Dec 3 2020, 5:22 PM

Make it less dependent on OS

Stan updated this revision to Diff 14369.Dec 3 2020, 5:34 PM

Fix command

Stan updated this revision to Diff 14373.Dec 3 2020, 6:34 PM

Fix typo

Stan updated this revision to Diff 14374.Dec 3 2020, 7:34 PM

Fix redirection

This revision was not accepted when it landed; it landed in state Needs Review.Dec 4 2020, 3:39 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 4 2020, 3:39 PM