Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

change abs to fabs #87

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tsky1971
Copy link

change abs to fabs to avoid error message (VC2015)
add cast on test return

@mbabuskov
Copy link

Function in NatPunchthroughClient.cpp could simply be changed to return const char * instead of casting on every return. The returned char array is never modified, so const would be a better solution.

@Luke1410
Copy link

Luke1410 commented Apr 2, 2018

The changes suggested to CCRakNetSlidingWindow.cpp we resolved in SLikeNet in the way suggested in #64 (was integrated in SLikeNet 0.1.0). IMO that solution is standard compliant and there's no need to use fabs there (note that obviously for C it would be a different matter --- pls state, if you think otherwise so we can reconsider our choice).

The changes (cast of string literals to non const pointers) we rejected. IMO you'd really not cast these into non-const pointers. We therefore went with the alternative also suggested by @mbabuskov (simply changing the return value) which is covered by #22 and was also incorporated in SLikeNet 0.1.0 already.

The change suggested to RakString::ToWideChar() is more problematic. It doesn't suffice to simply cast the returned empty string literal into a non-const wchar. The problem here is rather a questionable API design decision to return a non-const pointer and leaving it with the caller to free the allocated string again. The proper fix therefore would be to explicitly allocate an empty string. Thanks for your pointer here, this will be resolved in SLikeNet 0.2.0 (changes are already available on the master branch - internal case number SLNET-190). We'll also consider changing the API for this function in SLikeNet 2.0.0 to return a const pointer and no longer require the caller to free the returned pointer (internal case number SLNET-191).

Your changes to CMakeList.txt we tested but rejected. In our tests (CMake 3.7.2 / VS 2015) the generated project file is incorrect when omitting the quotes (i.e. only the first lib would be added to the nodefaultlib-list. Did you see different results when testing your changes to the cmake file? Was there a specific issue which led you to these changes? Unless you point out some issue with the test we did, we won't integrate that part of your pull request.

@Luke1410
Copy link

Luke1410 commented May 8, 2018

Since we did an unplanned release of SLikeNet, we decided to incorporate the fix for SLNET-190 in SLikeNet 0.1.2 already which is available now on https://www.slikenet.com/ and on the GitHub project page: https://github.com/SLikeSoft/SLikeNet/releases/tag/v.0.1.2 .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants