Fix coverity warnings for new SoundSources #569
Conversation
*** CID 61970: Uninitialized members (UNINIT_CTOR)
/src/sources/soundsourcesndfile.cpp: 17 in Mixxx::SoundSourceSndFile::SoundSourceSndFile(QUrl)()
11 return list;
12 }
13
14 SoundSourceSndFile::SoundSourceSndFile(QUrl url)
15 : SoundSource(url),
16 m_pSndFile(NULL) {
>>> CID 61970: Uninitialized members (UNINIT_CTOR)
>>> Non-static class member field "m_sfInfo.seekable" is not initialized in this constructor nor in any functions that it calls.
17 }
18
19 SoundSourceSndFile::~SoundSourceSndFile() {
20 close();
21 }
*** CID 61968: Uninitialized members (UNINIT_CTOR)
/src/sources/soundsourcemp3.cpp: 163 in Mixxx::SoundSourceMp3::SoundSourceMp3(QUrl)()
157 m_fileSize(0),
158 m_pFileData(NULL),
159 m_avgSeekFrameCount(0),
160 m_curFrameIndex(kFrameIndexMin),
161 m_madSynthCount(0) {
162 m_seekFrameList.reserve(kSeekFrameListCapacity);
>>> CID 61968: Uninitialized members (UNINIT_CTOR)
>>> Non-static class member field "m_madSynth.pcm" is not initialized in this constructor nor in any functions that it calls.
163 }
164
165 SoundSourceMp3::~SoundSourceMp3() {
166 close();
167 }
|
Since the SoundSource API allows close and then sub-sequent re-open it's important that we keep initializing the resources in Since I think you could fix this by just initializing the resources to zero in the constructor to satisfy Coverity. |
|
Calling the _init() functions twice in the constructor and in tryOpen() is not correct, because the invocations of _init() and _finish() should match exactly. I'll try to find a solution that is both correct and satisfies Coverity ;) |
|
Slight misunderstanding ;) Anyway, calling init()/finish() symmetrically and documenting this decision explicitly is even nicer and safer than a simple memset hack. |
|
If you do the equivalent of what tryOpen does (i.e. call the _init methods) then when the SoundSource is opened it will first be closed which clears them -- so it should be matched pairs of init/close if you do it that way (we were calling a close without a corresponding init before). |
|
Well, this is why I usually prefer managed resources ;) Here is an overview: The constructor is calling init() for initialization purposes. This must be matched by a destroy() in the constructor. But the constructor also needs to call close(), because close() does more than just destroying the decoder. Since close() might be called multiple times the invocations of init() and destroy() in close() must be balanced (please see the comments). This is the reason why the destructor finally has to call destroy() once again. |
|
thanks :) lgtm |
Fix coverity warnings for new SoundSources
Try to fix some minor Coverity warnings for 2 SoundSources by moving the initialization from tryOpen() into the constructor.
For details please refer to the individual commit messages that contain the Coverity warnings.