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

Replace raw new/delete in Util with stack object #11387

Merged
merged 1 commit into from Jan 7, 2017

Conversation

@thebrid
Copy link
Contributor

commented Jan 6, 2017

Description

Replace a single heap object allocated using new/delete with a stack-allocated object.

Motivation and Context

Raw new/delete may lead to inadvertent memory leaks if we return early / throw an exception. I considered using a smart pointer but the object we're constructing seemed relatively small so it seemed fine to just construct it on the stack.

How Has This Been Tested?

This concerns loading of SMI subtitles. I compiled & ran my changes on x86-64 Ubuntu and loaded a sample SMI subtitle file. KODI ran fine & displayed the expected subtitles.

The changes should be localised in this part of the code.

I considered adding unit tests for this change but this function has external dependencies on the filesystem which seem non-trivial to isolate.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@ace20022 ok with you?

@ace20022

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

+1 in general.

if we return early / throw an exception

I have not checked it, but where can this happen?

@thebrid

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

@ace20022 , I didn't have a specific case in mind, more general exception safety, but we could emit an exception here:

CUtil::ScanForExternalSubtitles
CDVDSubtitleStream::Open
CFileItem::CFileItem
std::string::operator=

@Paxxi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

Even if we don't care about exceptions we should not use raw pointers anymore except for non-owning use or possibly special case. This is cleanup that we could benefit from throughout our code base.
Good initiative @thebrid

@ace20022

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

jenkins build this please

@ksooo

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@thebrid nice. thanks for your contribution.

@ace20022 ace20022 merged commit 5f6fc55 into xbmc:master Jan 7, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@thebrid

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

Thanks 😄

@MartijnKaijser MartijnKaijser modified the milestone: L 18.0-alpha1 Jan 8, 2017
@thebrid thebrid deleted the thebrid:cpp11 branch Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.