Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix free/delete inconsistencies in OSystem_Win32::convertEncoding #1919

Merged
merged 2 commits into from Nov 8, 2019

Conversation

@criezy
Copy link
Member

criezy commented Nov 7, 2019

The current implementation of OSystem_Win32::convertEncoding causes some arrays allocated with new [] being freed using free() and not delete []. This is an attempt to fix this.

Since I am not even able to compile the code, not being on Windows, I would appreciate if others could review and try this before this is pushed to master.

This commit also removes what appeared to be an unused variable (wString) and adds a sanity check as the result of Win32::ansiToUnicode could be null.

@sluicebox

This comment has been minimized.

Copy link
Member

sluicebox commented Nov 7, 2019

I stepped through the changes in both codepaths on Windows with several test inputs and they are both fixed and doing what's intended.

The comment "Win32::unicodeToAnsi ses new to allocate" is missing the "u" and the project's code conventions are for no space in "delete []", which I only know from having just done this =)

return result;
if (!result)
return nullptr;
// Win32::unicodeToAnsi ses new to allocate the memory. We need to copy it into an array

This comment has been minimized.

Copy link
@SupSuper

SupSuper Nov 8, 2019

Contributor

Typo

@SupSuper

This comment has been minimized.

Copy link
Contributor

SupSuper commented Nov 8, 2019

Compiles fine here. Shame we can't just wrap them in managed Strings and avoid manual allocation, since they're Win32 types.

@criezy criezy force-pushed the criezy:delete-is-not-free branch from d4500d4 to adc49d5 Nov 8, 2019
@criezy

This comment has been minimized.

Copy link
Member Author

criezy commented Nov 8, 2019

Thank you to both of you. I have fixed the typo and delete[] and I will now merge this.

@criezy criezy merged commit 8ac0012 into scummvm:master Nov 8, 2019
0 of 2 checks passed
0 of 2 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.