#1642 MIDI file import/open crashes

[Mark R. Rubin]

Commit [ac2658] on Sun May 29 introduced a fatal crash when importing/opening MIDI files. The changes which cause the bug are:

 EventQuantizeCommand::EventQuantizeCommand(Segment &segment,
@@ -77,12 +79,15 @@ EventQuantizeCommand::EventQuantizeCommand(Segment &segment,
                                            QString settingsGroup,
                                            QuantizeScope scope):
     BasicCommand(getGlobalName(makeQuantizer(settingsGroup, scope)),
                  segment, startTime, endTime,
                  true),  // bruteForceRedo
+    m_quantizer(0),
     m_selection(nullptr),
-    m_settingsGroup(settingsGroup)
+    m_settingsGroup(settingsGroup),
+    m_progressTotal(0),
+    m_progressPerCall(0)
 {
     // nothing else -- m_quantizer set by makeQuantizer
 }

 EventQuantizeCommand::EventQuantizeCommand(EventSelection &selection,
@@ -91,12 +96,15 @@ EventQuantizeCommand::EventQuantizeCommand(EventSelection &selection,
     BasicCommand(getGlobalName(makeQuantizer(settingsGroup, scope)),
                  selection.getSegment(),
                  selection.getStartTime(),
                  selection.getEndTime(),
                  true),  // bruteForceRedo
+    m_quantizer(0),
     m_selection(&selection),
-    m_settingsGroup(settingsGroup)
+    m_settingsGroup(settingsGroup),
+    m_progressTotal(0),
+    m_progressPerCall(0)
 {
     // nothing else -- m_quantizer set by makeQuantizer
 }

Specifically, the m_quantizer(0) initializers wipe out what was just set by makeQuantizer(), in turn called so that getGlobalName() can be used to initialize the BasicCommand base class. Merely removing the lines fixes the problem, although a less convoluted initialization scheme might be considered.

I discovered this bug during testing in my feature request branches and thought it must be a side effect of the many changes I've made there. After tracking down the cause I belatedly checked and found it was also in master. I don't have the time to branch/fix/commit/push/merge-request but hope someone else will. As much as ac2658's "cppcheck updates" were and are a good idea, this bug illustrates the well-known problems caused by not having a good set of automated regression tests. Unfortunately that's another, much larger, issue that I don't have time to address.

[Philip Leishman]

Wow - thanks for finding that!!!
Just blindly following cppcheck can cause problems !
please merge my bug-1632-cppcheck branch again (and maybe keep the merge request open - still more to come)

[Ted Felix]

Yeah, cppcheck is really helpful, but you have to be careful and regression test. And that can be a huge headache in some areas. That's why I tend to avoid cppcheck, or just use it on small areas that I'm familiar with.

It's early in this cycle, so it's reasonably safe to do some of this work. The problems should be discovered before the next delivery.

As for the merge request, feel free to reopen, or just mention the branch in the bug report. That's good enough for me. I just fetch from your repo and see what's new periodically.

[Philip Leishman]

Right. Most of the cppcheck changes are fairly trivial. I will try to be more careful !!!
That's fine if you can merge my bug-1632-cppcheck branch when convenient.

[Ted Felix]

Merged as [1979b2]. Please test latest git.