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

Fixed unnecessary transfer of the ownership before release which returns the pointer itself #11726

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

JoergAtGithub
Copy link
Member

These were reported as resoource leak by Coverity Scan, not real issues, but also an unnecessary commands. So this is the cleanest way to silence the warning.
While going through these warnings I noticed also #11725 which looks similar, but is a real ressource leak.

// pTag we need to release the ownership to avoid double deletion!
pFrame.release();

// Release returns a pointer to the managed object and releases the ownership,
Copy link
Contributor

@uklotzde uklotzde Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is obsolete after dropping the 2-step get-release pattern. Repeating the std definition of release() is pointless and confusing. Also applies to other locations in the code.

The 2-step pattern is required for exception safe C++ programming. But since any exception thrown by addFrame() is expected to be fatal a resource leak doesn't matter then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code has been written with purpose and the comment explained why. If you decide to take a simpler approach then delete the obsolete comment. I don't care, does not really matter. But please change it in a consistent way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resource leak reported by Coverity is obviously a false positive.

Comment on lines 386 to 387
// Release returns a pointer to the managed object and releases the ownership,
// the plain pointer in pFrame is owned and managed by pTag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this should be sufficient

Suggested change
// Release returns a pointer to the managed object and releases the ownership,
// the plain pointer in pFrame is owned and managed by pTag
// pTag takes the ownership of pFrame

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you

@daschuer daschuer merged commit 363b0cd into mixxxdj:main Jul 10, 2023
@JoergAtGithub JoergAtGithub deleted the FixCoverityIssues branch July 10, 2023 06:42
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.

3 participants