#1632 cppcheck report

[Martin Strunz]

/src/sound/JackDriver.cpp line 2460

Code:

bool
JackDriver::closeRecordFile(InstrumentId id,
                            AudioFileId &returnedId)
{
    if (m_fileWriter) {
        return m_fileWriter->closeRecordFile(id, returnedId);
        if (m_fileWriter->running() && !m_fileWriter->haveRecordFilesOpen()) { // <- unreachable
            m_fileWriter->terminate();
        }
    } else
        return false;
}

[Philip Leishman]

Another nice little bug.
I would have hoped that the gnu c++ compiler would warn about this - but no :(
So looking for static code analysis tools I found cppcheck - easy to use.
with the command cppcheck --enable=all --inconclusive --std=c++11 --quiet src in the rosegarden directory I found this bug and lot of others.

Unreachable code:
[src/base/SegmentNotationHelper.cpp:789]: (style, inconclusive) Statements following return, break, continue, goto or throw will never be executed.
[src/gui/application/RosegardenMainWindow.cpp:8399]: (style, inconclusive) Statements following return, break, continue, goto or throw will never be executed.
[src/gui/editors/eventlist/TrivialVelocityDialog.cpp:53]: (style) Statements following return, break, continue, goto or throw will never be executed.
[src/gui/editors/notation/NotationSelector.cpp:647]: (style, inconclusive) Statements following return, break, continue, goto or throw will never be executed.
[src/gui/editors/notation/NotationStrings.cpp:270]: (style) Statements following return, break, continue, goto or throw will never be executed.
[src/gui/editors/notation/NoteCharacter.cpp:111]: (style) Statements following return, break, continue, goto or throw will never be executed.
[src/sound/AlsaDriver.cpp:4251]: (style, inconclusive) Statements following return, break, continue, goto or throw will never be executed.
[src/sound/JackDriver.cpp:2458]: (style) Statements following return, break, continue, goto or throw will never be executed.

And lots of other interesting things. I have appended the output.

[Martin Strunz]

Perfect !
Maybe gcc with option -Wunreachable-code works

[Philip Leishman]

I tried -Wunreachable-code. Still no warning !!??!!
So my suggestion - we rename this bug to "cppcheck clean up" or something like that and try to fix all these issues. I never like code warnings and these seem to qualify as warnings for me.

[Martin Strunz]

sounds ok to me.

[Ted Felix]

See also [#1344]. And if you really want to dig into some static code analysis of Rosegarden beyond cppcheck, see this rather condescending site (Russian sense of humor?):

https://pvs-studio.com/en/blog/posts/cpp/0536/

They offer a one year key for open source projects. Seems like a solid tool. I didn't have time to care back then. Far more serious bugs to fix than these minor ones. But if anyone is interested and doesn't mind installing software from Putin-land....

[Philip Leishman]

There was a bug with this title 10 years ago. A lot happened since then !!

[Philip Leishman]

I have started on this. Most of the changes are fairly trivial but I think they are an improvement.
Oh and I created a cmake target. make cppcheck will create/update a file cppcheck.out in the build directory (if cppcheck is installed).
I also included the --inline-suppr option. This means warnings can be suppressed with a comment in the code. See eg. Event.h:
// cppcheck-suppress functionConst
cppcheck falsely claims this function can be const !!
See merge request.

[Ted Felix]

Some of these commits introduce endless recursion loops. E.g. the const_iterator version of Segment::findTime() now calls itself.

I'm going to cherry-pick each one into a new branch and fix the endless loops along the way so we can bisect through here safely in the future. This will make a mess of your branch, so you'll want to start another for continued work on this.

Should be done shortly....

[Philip Leishman]

Hmmm.. I am a bit confused by this.
Google tells me that c++ methods can not be overloaded by return vaule but (even without my change) that is what we are doing here.
A question: How do you get the infinite recursion - I have tried a lot of things and I have had no problem
Suggestion: maybe remove the const_iterator findTime method and make iterator findTime const ?

[Ted Felix]

I'm guessing it's a compiler version difference. I end up in an endless loop blowing over the stack the moment I press play. Those routines are called constantly (pardon the pun 🤣).

Regardless, this is just bad code at such a fundamental level. There's a simple solution that is used by the STL to fix this... Don't use function overloading (e.g. STL's begin() and cbegin()). Anyway, I've found what appear to be all the issues. I'm reverting those lines and adding comments recommending a solution.

Almost done...

[Ted Felix]

Just pushed all this as [0c975c] with some modifications. Please test latest git.

Hopefully I didn't miss anything. Found one commit that was a merge which tripped me up a little, but git handled it once I figured out how. I think if you run cppcheck again, you'll probably immediately see if I missed anything significant.

Search on "in an endless" to find the 5 endless loop areas. I think I'll tackle those next as recommended in the comments.

[Ted Felix]

Pushed final cleanup as [c0856e]. Please test latest git.

Turns out only one of the problematic functions needed to reference/reuse the non-const version: MidiDevice::getControlParameterConst(). The rest were either unused, or easily simplified. Went with a "Const" suffix to clarify the names and avoid function overloading. Should pass cppcheck. If not, we can always put an exception in.

[Philip Leishman]

Strange that I didn't get the run loop. as you say maybe a compiler issue!
Looks good to me - Definitely cleaner code !!
Still a lot of (over 1000) cppcheck errors to go. I will continue and update the merge request when more is done.

[Philip Leishman]

I am proceeding with the cppcheck corrections. Please merge when convenient.
I have just run into some unreachable code in TrivialVelocityDialog after the return in getVelocity. I tested - create and edit a trigger segment and press edit velocity.
This certainly seems to be broken. Surely the button box should be added in the constructor and not after the return getVelocity
Any thoughts ???

[Philip Leishman]

OK - so I moved the unreachable code into the constructor and adjusted the max value of the spin box.
Seems to be usable now !!
Please merge

[Ted Felix]

Pushed latest as [d14350]. Please test latest git.

Your fixed TrivialVelocityDialog looks good to me. Not sure what it does, but I managed to get it to come up with the triggered segment stuff and the buttons work as advertised.

[Philip Leishman]

I guess that button hasn't been used much in the past 10 years ;)

So I have run into the next unreachable code in NoteCharacter.cpp. This seems to be some old code which never runs. It seems m_rep is never used in the class. Oh careful - there is a friend class declaration (I am not keen on using friend classes when avoidable) ! No the friend class NoteFont doesn't use m_rep either.
If nobody has a different suggestion I will get rid of the old code, remove m_rep and maybe try to get rid of the friend declaration.
Ideas ?

[Ted Felix]

I searched on m_rep and it only occurs within NoteCharacter. So sounds good to remove. I have no idea what the other code is about. Looks like experiments during an upgrade of Qt.

I'm not fond of friend either. Looks like it's just there to allow NoteFont to use the ctor that is immediately under it. I think we can move the ctor to public and be done with it. Without comments to justify the private/friend approach, I consider it open season.

Sometimes friend is helpful when preserving strict divisions within a design. But it's something of a sledgehammer driving a thumbtack since it gives access to everything.

[Philip Leishman]

OK. Did a bit of cleaning up. Still seems to work OK !!
Please merge,

[Ted Felix]

Latest pushed as [850ae2]. Please test latest git.

Please review the last two there. I had a compilation error due to the explicit SegmentRect ctor.

[Philip Leishman]

Oh - strange. So you have a compile error with the latest commit on master ?
Can you post the error.
I have no problem compiling. It seems the single argument constructor is only used once in CompositionModelImpl.cpp at line 246. This seems to be legitimate code even with the explicit declaration (which is meant to avoid unwanted type conversions).