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

Various cppcheck perf fixes #6803

Merged
merged 1 commit into from Jul 3, 2015

Conversation

Projects
None yet
9 participants
@tobbi
Contributor

tobbi commented Mar 23, 2015

Another round of cppcheck perf fixes throughout various files.

I picked issues at random and fixed them.

@ksooo

This comment has been minimized.

Member

ksooo commented Mar 24, 2015

Looking good.

@mkortstiege

This comment has been minimized.

Member

mkortstiege commented Mar 24, 2015

Please compile and run-time test your changes ;) This breaks build: class ‘CGUIDialogSubtitles’ does not have any field named ‘m_loadType’

@Montellese

This comment has been minimized.

Member

Montellese commented Mar 24, 2015

@mkortstiege: It does have an m_loadType field but it is inherited from CGUIWindow so it can't be set in the initialization list.

@mkortstiege

This comment has been minimized.

Member

mkortstiege commented Mar 24, 2015

@Montellese sure. Just pasted the error output.

@fritsch

This comment has been minimized.

Member

fritsch commented Mar 24, 2015

Basically I don't like the new calling in the initializer list ... http://stackoverflow.com/questions/14681714/using-new-in-a-member-initializer-list-of-constructor <- might become harmful

@Paxxi

This comment has been minimized.

Member

Paxxi commented Mar 24, 2015

Agree with @fritsch , it's unlikely but best avoid a bad habit.

@ksooo

This comment has been minimized.

Member

ksooo commented Mar 24, 2015

+1 to what @fritsch said. new in initlist might lead to resource leaks (due to std::bad_alloc), overlooked this in the frist place, sorry.

@ksooo

This comment has been minimized.

Member

ksooo commented Mar 24, 2015

To solve the "new in initializer list may lead to resource leak" problem I suggest to change the type of m_subtitles and m_serviceItems to std::auto_ptr.

Then, new in init list is no harmful any more. If first new (m_subtitles) in initlist fails, no memory gets allocated on the heap, thus no resource leak. If first new succeeds and second new (m_serviceItems) fails, dtor of (the already fully constructed) std::auto_ptr m_subtitles will be called, which frees the heap memory allocated with the first new.

@Paxxi

This comment has been minimized.

Member

Paxxi commented Mar 24, 2015

Well not auto_ptr that's deprecated, unique_ptr or shared_ptr depending on ownership

@ksooo

This comment has been minimized.

Member

ksooo commented Mar 24, 2015

@Paxxi sure. thx for the headsup.

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented May 23, 2015

care to update and rebase? thx

@tobbi

This comment has been minimized.

Contributor

tobbi commented May 23, 2015

I'm in the middle of my Bachelor's thesis. I will put it on my list to do when I have got time :)

@tobbi

This comment has been minimized.

Contributor

tobbi commented Jun 20, 2015

I rebased the contents of this PR against latest master... Note that cppcheck found other things but I didn't include them in this PR yet. I might do another PR in the future that takes care of the things found in the meantime.

@tobbi

This comment has been minimized.

Contributor

tobbi commented Jun 20, 2015

Oh, wait, before we hit any issues, I will compile-check it. ;)

@hudokkow

This comment has been minimized.

Member

hudokkow commented Jun 20, 2015

jenkins build this please

@tobbi

This comment has been minimized.

Contributor

tobbi commented Jun 23, 2015

Please try to build again (I moved the m_loadType initialization back to the c'tor body)

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jun 23, 2015

jenkins build this please

@tobbi

This comment has been minimized.

Contributor

tobbi commented Jun 24, 2015

Can you please trigger another build? I will probably set up a VM just for building Kodi, because it's just tiresome on OS X and doesn't work.

@mkortstiege

This comment has been minimized.

Member

mkortstiege commented Jun 24, 2015

jenkins build this please

@tobbi

This comment has been minimized.

Contributor

tobbi commented Jun 24, 2015

win32 build failure seems unrelated! Please merge!

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jun 28, 2015

@mkortstiege merge for 15.0 or delay till next?

@mkortstiege

This comment has been minimized.

Member

mkortstiege commented Jun 28, 2015

No need for those in v15. Could go in once branched.

@mkortstiege mkortstiege added the Cleanup label Jul 2, 2015

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jul 3, 2015

jenkins build and merge

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 3, 2015

@jenkins4kodi jenkins4kodi merged commit 302c474 into xbmc:master Jul 3, 2015

1 check was pending

default Merged build started.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment