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

Plug a few memory leaks #1499

Merged
merged 14 commits into from Jul 11, 2014
Merged

Plug a few memory leaks #1499

merged 14 commits into from Jul 11, 2014

Conversation

adrientetar
Copy link
Member

…and fix indent along the way.

Diff without whitespace

r? @JoesCat

getFontForgeUserDir() allocates memory or returns NULL.
The NULL case is already handled, free the memory otherwise.

CR-fixed: 1225170
gwwv_ask_string is an alias to GWidgetAskString8 which uses the allocating
u2utf8_copy() internally.
If ret is non-null, free it at the end of the block.

CR-fixed: 1225167.
WordlistLoadFileToGTextInfo allocates or returns 0.
The zero case is already handled, so free **words otherwise at the end.
@adrientetar
Copy link
Member Author

By the way we should run GNU Indent through the entire codebase because right now it's kind of a pain to work with existing code.

@davelab6
Copy link
Member

On 11 July 2014 08:53, Adrien Tétar notifications@github.com wrote:

By the way we should run GNU Indent through the entire codebase because
right it's kind of a pain to work with existing code.

There's been some protest against that as it would lose git blame but
perhaps the tradeoff is worthwhile :)

@adrientetar adrientetar mentioned this pull request Jul 11, 2014
20 tasks
@adrientetar
Copy link
Member Author

There's been some protest against that as it would lose git blame but perhaps the tradeoff is worthwhile :)

You can ignore whitespace change in blame with the -w switch. But Indent would also change a bit the coding style (i.e., get rid of returns with parenthesis).

At this point in the function, glyphdir hasn't been initialized.
Don't try to free it.
@JoesCat
Copy link
Contributor

JoesCat commented Jul 11, 2014

@JoesCat You can merge this Pull Request by running:

I think I'll leave this one to you. The credit is all yours.

Viewing the large number of edits, I'm guessing you're finding the scan both interesting and useful. ;-)

@adrientetar
Copy link
Member Author

I'm guessing you're finding the scan both interesting and useful. ;-)

There's also a lot of overruns and memory corruptions listed there, like 30 of them or so... it's real bad.

adrientetar added a commit that referenced this pull request Jul 11, 2014
@adrientetar adrientetar merged commit 8c05b8e into fontforge:master Jul 11, 2014
@adrientetar adrientetar deleted the leaks branch July 11, 2014 14:59
@davelab6
Copy link
Member

BTW Frank Trampe has requested that all pull requests be actively reviewed
by at least one other developer, and has volunteered to do that :)

I've updated the CONTRIBUTING.md file to make this clearer:

6445fd0#diff-6a3371457528722a734f3c51d9238c13R30

@adrientetar
Copy link
Member Author

Good to know.

@frank-trampe
Copy link
Contributor

I have reviewed the now-merged changes, and I find them entirely to my liking.

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

Successfully merging this pull request may close these issues.

None yet

4 participants