Page MenuHomeWildfire Games

#1325 Replace ecvt() in FCollada with something else
Needs ReviewPublic

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

Details

Reviewers
Itms
Trac Tickets
#1325
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

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

NF created this revision.Nov 1 2019, 5:00 PM
smiley added a subscriber: smiley.Nov 2 2019, 11:55 AM
smiley 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.

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)

smiley 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+ ?