Potential fixes for 4 code quality findings#3702
Conversation
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
|
I don't think everything here is valid. |
| QSoundEffect sf; | ||
| sf.setSource ( QUrl::fromLocalFile ( ":sounds/res/sounds/new_message.wav" ) ); | ||
| sf.play(); |
There was a problem hiding this comment.
This may be valid. Stack vs heap allocation?
There was a problem hiding this comment.
No this won't work. The original does indeed have a memory leak, as the pointer returned from new QSoundEffect() is only local, and disappears at the end of the block. But I think changing it so that the QSoundEffect sf is allocated locally on the stack may cause a crash, or at least truncate the playback, as the object will be destroyed immediately at the end of the block while the sound is still playing.
The correct solution is actually the one shown below around line 910: connect a lambda to the playingChanged signal and call deleteLater() when it is finished with.
| QSoundEffect sf; | ||
| sf.setSource ( QUrl::fromLocalFile ( ":sounds/res/sounds/new_message.wav" ) ); | ||
| sf.play(); |
There was a problem hiding this comment.
No this won't work. The original does indeed have a memory leak, as the pointer returned from new QSoundEffect() is only local, and disappears at the end of the block. But I think changing it so that the QSoundEffect sf is allocated locally on the stack may cause a crash, or at least truncate the playback, as the object will be destroyed immediately at the end of the block while the sound is still playing.
The correct solution is actually the one shown below around line 910: connect a lambda to the playingChanged signal and call deleteLater() when it is finished with.
| QSoundEffect* sf = new QSoundEffect ( this ); | ||
| connect ( sf, &QSoundEffect::playingChanged, this, [sf]() | ||
| { | ||
| if ( !sf->isPlaying() ) | ||
| { | ||
| sf->deleteLater(); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
This looks ok and is the correct way to avoid the memory leak.
2 participants
This PR applies 4/4 suggestions from code quality AI findings.
Only for review. We should then decide if we actually have a memory leak here.