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 minor potential leaks on OS X and Dead Store in sf::Ftp #615

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mantognini
Member

mantognini commented May 25, 2014

This is really not a big deal but still.

Commit details:

  • Fixed potential memory leaks on OS X
  • And renamed create* methods to new* to follow more closely Objective-C
    naming convention regarding memory management.

Note that clang analyzer also spot a «dead store» when writing false to isInsideMultiline. Apparently the value is never read again. However, I think this is a false positive seen the if statement on line 419.

A few other dead stores are also reported in stb_image.h but that's not our job.

Fixed potential memory leaks on OS X
And renamed create* methods to new* to follow more closely Objective-C
naming convention regarding memory management.

@mantognini mantognini added OS X and removed confirmed labels May 25, 2014

@mantognini mantognini added this to the 2.2 milestone May 25, 2014

@mantognini mantognini self-assigned this May 25, 2014

@minirop

This comment has been minimized.

Show comment
Hide comment
@minirop

minirop May 26, 2014

line 442 IS a dead store since you don't read from isInsideMultiline before the return statement on line 464.

minirop commented May 26, 2014

line 442 IS a dead store since you don't read from isInsideMultiline before the return statement on line 464.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 26, 2014

Member

Yes, you're right! I missed that statement somehow...

Member

mantognini commented May 26, 2014

Yes, you're right! I missed that statement somehow...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

I assume this needs an update then.

Member

eXpl0it3r commented May 26, 2014

I assume this needs an update then.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 27, 2014

Member

Done. (well, the name of the branch doesn't match 100% its purpose... a fast-forward merge would be nice.)

Member

mantognini commented May 27, 2014

Done. (well, the name of the branch doesn't match 100% its purpose... a fast-forward merge would be nice.)

@mantognini mantognini changed the title from Fix minor potential leaks on OS X to Fix minor potential leaks on OS X and Dead Store in sf::Ftp May 27, 2014

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 28, 2014

Member

If you think the commits are okay, I'll merge it, given that we don't really have someone to review OS X code...

Member

eXpl0it3r commented May 28, 2014

If you think the commits are okay, I'll merge it, given that we don't really have someone to review OS X code...

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 28, 2014

Member

Yes i think it's good – at least the analyzer doesn't complain anymore and the profiler doesn't report bad memory access nor leaks.

Member

mantognini commented May 28, 2014

Yes i think it's good – at least the analyzer doesn't complain anymore and the profiler doesn't report bad memory access nor leaks.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 28, 2014

Member

Merged in 5912d20

Member

eXpl0it3r commented May 28, 2014

Merged in 5912d20

@eXpl0it3r eXpl0it3r closed this May 28, 2014

@eXpl0it3r eXpl0it3r added the resolved label May 28, 2014

@mantognini mantognini deleted the bugfix/osx-leaks branch May 28, 2014

@mantognini mantognini removed their assignment Apr 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment